Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Links attribute #480

Merged
merged 4 commits into from
Nov 9, 2020
Merged

Links attribute #480

merged 4 commits into from
Nov 9, 2020

Conversation

sitaktif
Copy link
Contributor

@sitaktif sitaktif commented Nov 6, 2020

  • Add the Cargo.toml links attribute to our rule to allow inferring depenv environment variable names correctly.
  • Replace the sandbox directory with ${pwd} in the values of the depenv environment variables.
  • Add missing transitive dependencies between build script targets.
  • Add an example to test this non-trivial case.

This allows building packages with sys transitive dependencies (e.g. when depending on crates such as git2).

@google-cla google-cla bot added the cla: yes label Nov 6, 2020
@illicitonion illicitonion self-requested a review November 6, 2020 10:24
illicitonion
illicitonion approved these changes Nov 6, 2020
View reviewed changes
Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Windows CI is failing - I'm guessing adding a windows condition to the openssl BUILD file which says shared libraries should be .dll may fix it? (Also Buildifier CI is failing)

@dfreese How do you feel about the openssl example? I think it's very useful to have...

examples/complex_sys/raze/remote/libgit2-sys-0.12.14+1.1.0.BUILD.bazel
"ssh",
"ssh_key_from_memory",
],
# LOCALLY MODIFIED
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other reviewers: These files are slightly modified from the cargo-raze generated versions, because they require attributes raze doesn't currently set. This feels like a pretty reasonable approach, as if people re-gen the git diff will show the local modifications to re-apply (at least, until Raze supports them)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue open on cargo-raze for what you ran into here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UebelAndre I opened the following two issues:

cargo/cargo_build_script.bzl
@@ -165,6 +167,10 @@ _build_script_run = rule(
"crate_name": attr.string(
doc = "Name of the crate associated with this build script target",
),
"links": attr.string(
default = "",
doc = "The name of the native library this crate links against.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you run docs/update_docs.sh to update the docs?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is default = "", necessary?

@sitaktif
Copy link
Contributor Author

sitaktif commented Nov 6, 2020

Making an attempt at fixing the windows build (using CI as I don't have a Windows machine handy).

UebelAndre
UebelAndre reviewed Nov 6, 2020
View reviewed changes
cargo/cargo_build_script.bzl
@@ -165,6 +167,10 @@ _build_script_run = rule(
"crate_name": attr.string(
doc = "Name of the crate associated with this build script target",
),
"links": attr.string(
default = "",
doc = "The name of the native library this crate links against.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is default = "", necessary?

examples/complex_sys/Cargo.toml Outdated
[raze]
workspace_path = "//complex_sys/raze"
genmode = "Remote"
incompatible_relative_workspace_path = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line should not be necessary anymore. The default is true now

@sitaktif
depenv: use the links attribute when set
6184835 
The name of the environment variables present in the `.depenv` files for
sys packages should not be inferred from the package name but from the
`links` Cargo.toml attribute instead, like cargo does.

For backward-compatibility reasons, we fall back to inferring the
value from the name if the `links` attribute is unset.
@sitaktif
Include dep build script out dirs in cargo_build_script inputs
03951f8
@sitaktif
Add openssl build script example
0425361
@sitaktif
Regenerate documentation
1ddf51f
@sitaktif
Copy link
Contributor Author

sitaktif commented Nov 9, 2020

@UebelAndre @dae @illicitonion I think I addressed your comments, PTAL!

@sitaktif sitaktif mentioned this pull request Nov 9, 2020
Closed
@illicitonion illicitonion merged commit ab1756e into bazelbuild:master Nov 9, 2020
@sitaktif
Copy link
Contributor Author

sitaktif commented Nov 9, 2020

Thanks!

@sitaktif sitaktif deleted the links-attribute branch November 9, 2020 14:51
@sitaktif sitaktif mentioned this pull request Nov 12, 2020
Merged
@UebelAndre
Copy link
Collaborator

@sitaktif Hey, sorry to ping this out of the blue, I was trying to regenerate the outputs of the complex_sys example using cargo-raze version 0.8 but couldn't get it to compile. This does indeed seem complex since you're building OpenSSL 😅. If you have time in the future, could I get you to generate a lockfile and update the [raze] settings so future re-runs won't break this? 🙏

@sitaktif
Copy link
Contributor Author

sitaktif commented Jan 8, 2021

@UebelAndre ACK - looking to fix that right now 👍

@sitaktif sitaktif mentioned this pull request Jan 8, 2021
Merged
@sitaktif
Copy link
Contributor Author

sitaktif commented Jan 8, 2021

I opened #549 to address (assuming I understood the issue!)

illicitonion added a commit to illicitonion/cargo-raze that referenced this pull request Mar 18, 2021
@illicitonion
Set links attribute
775424f 
This was added to rules_rust in
bazelbuild/rules_rust#480 and we started parsing
out the metadata in cargo_raze in google#281 but didn't start populating it in
generated BUILD files.

This enables us to remove a hack in rules_rust where we try to infer the
correct value for the `links` attribute if it wasn't set, based on the
name, which in turn will allow us to remove the `crate_name` attribute
completely.
@illicitonion illicitonion mentioned this pull request Mar 18, 2021
Merged
acmcarther pushed a commit to google/cargo-raze that referenced this pull request Mar 18, 2021
@illicitonion
Set links attribute (#400)
2ed7d14 
This was added to rules_rust in
bazelbuild/rules_rust#480 and we started parsing
out the metadata in cargo_raze in #281 but didn't start populating it in
generated BUILD files.

This enables us to remove a hack in rules_rust where we try to infer the
correct value for the `links` attribute if it wasn't set, based on the
name, which in turn will allow us to remove the `crate_name` attribute
completely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UebelAndre UebelAndre UebelAndre left review comments

@illicitonion illicitonion illicitonion approved these changes

@dae dae dae left review comments

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sitaktif @UebelAndre @dae @illicitonion