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 deps #929

Merged
merged 7 commits into from
Sep 10, 2021
Merged

Update deps #929

merged 7 commits into from
Sep 10, 2021

Conversation

illicitonion
Copy link
Collaborator

No description provided.

@UebelAndre
Copy link
Collaborator

I think this is dropping the binary_deps feature which reintroduces dependencies/complications around OpenSSL. Is that intentional?

@illicitonion
Copy link
Collaborator Author

Yes - I don't want to maintain a long-term fork, and I want to start pulling in cargo-raze updates, so if we want to remove those features we should work on pulling out a reusable core library :)

@illicitonion illicitonion force-pushed the update-deps branch 14 times, most recently from aad256b to bcfe7a4 Compare September 7, 2021 11:05
crate_universe/bootstrap.bzl Outdated Show resolved Hide resolved
@@ -55,12 +55,17 @@ def cargo_bootstrap(
if build_mode == "release":
args.append("--release")

env = {}

if repository_ctx.attr.include_perl_on_windows and "win" in repository_ctx.os.name:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of always using setting PERL here, could the cargo_bootstrap_repository rule take an env section which allows users to set these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a windows_env_vars_to_labels which does this just on Windows, but it feels still pretty similarly specific...

Copy link
Collaborator

Choose a reason for hiding this comment

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

windows_env_vars_to_labels to me is an improvement but not quite what I had in mind. Would something like #932 work for you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Works for me! Though exactly which triple I should be using for "Whatever CI is on Windows" I'm not sure :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd probably just use:

x86_64-pc-windows-gnu
x86_64-pc-windows-msvc

Perhaps that section could be expanded to support matching on components of a triple, system, vendor, etc... Host detection is not the easiest thing to do sadly... I wish there was more in Bazel to make this easy.

@illicitonion illicitonion force-pushed the update-deps branch 4 times, most recently from 59a8ce6 to c4e013b Compare September 10, 2021 14:25
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

One last question 😅

@@ -223,6 +225,7 @@ tasks:
platform: windows
environment:
RULES_RUST_CRATE_UNIVERSE_BOOTSTRAP: true
OPENSSL_STATIC: "1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this potentially be passed as an env attribute to the bootstrap definition to avoid having two places where environment variables are set?

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Two more comments but I leave it up to you to decide what to do with them. Looks good to me! Thanks for updating this 🙏

cargo/cargo_bootstrap.bzl Outdated Show resolved Hide resolved
illicitonion and others added 2 commits September 10, 2021 17:34
Co-authored-by: UebelAndre <github@uebelandre.com>
@illicitonion illicitonion merged commit 20cec2e into bazelbuild:main Sep 10, 2021
@illicitonion illicitonion deleted the update-deps branch September 10, 2021 18:09
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

2 participants