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.

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

Update complex_sys example cargo-raze outputs #549

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

sitaktif
Copy link
Contributor

@sitaktif sitaktif commented Jan 8, 2021

Added a lockfile and updated the Cargo.toml with cargo-raze 0.8 stanzas.

Then, re-generated build files with raze 0.8.0, using cargo raze in
the example directory.


CC @UebelAndre as a follow-up to #480 (comment)

@sitaktif
Copy link
Contributor Author

sitaktif commented Jan 8, 2021

(rebased onto latest master)

@@ -0,0 +1,22 @@
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is masking the BUILD file that has the target defined in it. This is likely a side effect of having run cargo-raze without package_aliases_dir set to "raze".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, thanks for spotting this, and you're right, that's exactly what happened. I assumed it was a new build file generated by cargo-raze and didn't even try to make sense of it, sorry about that.

Now I can see the error you hit earlier, I'll work on fixing it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it helps, I'm also trying to build openssl-sys for my PR here: google/cargo-raze#340 (see //third_party/openssl/... and //third_party/cargo/remote/BUILD.openssl-sys-0.9.60.bazel

I got it to work on MacOS but that's only because the build.rs script was able to find openssl on my machine and it wasn't using the target I wanted it to. Been trying to get back to that but haven't had the time. Hoping to find the time this weekend 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, so I was looking at the diff of the complex_sys example build scripts (before and after running cargo-raze) and got reminded that cargo-raze does not currently handle a couple of attributes that are needed for openssl to work.

Namely, these two manual changes are needed here:

examples/complex_sys $ grep '# MODIFIED MANUALLY #' -A5 -r raze
raze/remote/BUILD.libssh2-sys-0.2.20.bazel:    # MODIFIED MANUALLY #
raze/remote/BUILD.libssh2-sys-0.2.20.bazel-    links = "ssh2",
raze/remote/BUILD.libssh2-sys-0.2.20.bazel-    data = glob(["**"]) + [
raze/remote/BUILD.libssh2-sys-0.2.20.bazel-        "@openssl",
raze/remote/BUILD.libssh2-sys-0.2.20.bazel-    ],
raze/remote/BUILD.libssh2-sys-0.2.20.bazel-    # / MODIFIED MANUALLY #
--
raze/remote/BUILD.openssl-sys-0.9.60.bazel:    # MODIFIED MANUALLY #
raze/remote/BUILD.openssl-sys-0.9.60.bazel-    build_script_env = {
raze/remote/BUILD.openssl-sys-0.9.60.bazel-        "OPENSSL_DIR": "../openssl/openssl",
raze/remote/BUILD.openssl-sys-0.9.60.bazel-    },
raze/remote/BUILD.openssl-sys-0.9.60.bazel-    # / MODIFIED MANUALLY #
raze/remote/BUILD.openssl-sys-0.9.60.bazel-    crate_features = [
--
raze/remote/BUILD.openssl-sys-0.9.60.bazel:    # MODIFIED MANUALLY #
raze/remote/BUILD.openssl-sys-0.9.60.bazel-    data = glob(["**"]) + [
raze/remote/BUILD.openssl-sys-0.9.60.bazel-    "@openssl",
raze/remote/BUILD.openssl-sys-0.9.60.bazel-    ],
raze/remote/BUILD.openssl-sys-0.9.60.bazel-    # / MODIFIED MANUALLY #
raze/remote/BUILD.openssl-sys-0.9.60.bazel-    edition = "2015",

They correspond to:

  • adding data to the build script of a crate (would need a new Cargo.toml raze meta.package.* attribute)
  • adding env vars to the build script of a crate (would need a new Cargo.toml raze meta.package.* attribute)
  • the links attribute which needs to be parsed from the Cargo.toml

We do need this openssl example to be tested here in rules_rust so we need to keep the manual changes around for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, you should also take a look at google/cargo-raze#343 and (now successfully building) google/cargo-raze#340. google/cargo-raze#340 builds openssl-sys, libgit2-sys, libssh2-sys, and curl-sys in order to successfully build cargo-raze with Bazel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's great, one nice step forwards!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we'll still be missing another couple of attributes so maybe we can merge the changes are they are now and "razificate" in subsequent PRs? I unfortunately don't have too much spare time at the moment...

Copy link
Collaborator

@UebelAndre UebelAndre Jan 11, 2021

Choose a reason for hiding this comment

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

I think so long as everything is locked, merging should be fine. Then it's simply a matter of making sure cargo-raze can generate BUILD files with the same information.

Added a lockfile and updated the Cargo.toml with cargo-raze 0.8 stanzas.

Then, re-generated build files with raze `0.8.0`, using `cargo raze` in
the example directory.

Finally, a couple of attributes needed to be set manually because
cargo-raze doesn't support them yet. One can grep for "MODIFIED
MANUALLY" in the generated BUILD files.
@illicitonion illicitonion merged commit 570b4a1 into bazelbuild:master Jan 12, 2021
@sitaktif sitaktif deleted the complex-sys-raze-0.8 branch January 12, 2021 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants