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

Support additional registries #412

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

illicitonion
Copy link
Collaborator

@illicitonion illicitonion commented Apr 13, 2021

This relies on the registry information being in .cargo/config.toml (and
will proactively error if other keys are found in there).

The reason for this is that we need to run cargo metadata before we
even parse out config, and we can't run cargo metadata without having
this data active for that cargo invocation. It also simplifies
implementation a lot, by just copying around one file rather than
needing to generate config for cargo to use.

Fixes #24.

Copy link
Member

@acmcarther acmcarther left a comment

Choose a reason for hiding this comment

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

Generally lg, but i feel somewhat strongly that this should not fail on unknown fields.

impl/src/cargo_config.rs Outdated Show resolved Hide resolved
@illicitonion
Copy link
Collaborator Author

@acmcarther I addressed the comments - could I nudge you towards a merge? :) Thanks!

@illicitonion
Copy link
Collaborator Author

@acmcarther Friendly ping? :)

@illicitonion illicitonion force-pushed the additional_registries branch 2 times, most recently from 9d036eb to 1515b3f Compare August 25, 2021 10:47
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.

This looks good to me 😄. A couple of small comments and can you run bazel run //tools:bootstrap && bazel run //tools:examples_raze && bazel test //... locally just to make sure nothing changed about the generated files?

impl/src/planning/subplanners.rs Show resolved Hide resolved
impl/src/planning/subplanners.rs Outdated Show resolved Hide resolved
This relies on the registry information being in .cargo/config.toml (and
will proactively error if other keys are found in there).

The reason for this is that we need to run `cargo metadata` before we
even parse out config, and we can't run `cargo metadata` without having
this data active for that `cargo` invocation. It also simplifies
implementation a lot, by just copying around one file rather than
needing to generate config for `cargo` to use.

Raze will hard-error if a `.cargo/config.toml` is present containing
keys it doesn't understand.
@illicitonion
Copy link
Collaborator Author

This looks good to me 😄. A couple of small comments and can you run bazel run //tools:bootstrap && bazel run //tools:examples_raze && bazel test //... locally just to make sure nothing changed about the generated files?

I have done - the changes are included in this PR :) (Specifically adding the serde feature to url :))

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.

Thanks for doing the cleanup! Looks good to me 😄

@illicitonion illicitonion merged commit 97ae730 into google:main Aug 25, 2021
@illicitonion illicitonion deleted the additional_registries branch August 25, 2021 15:31
@UebelAndre
Copy link
Collaborator

@illicitonion Hey, I think this introduced a regression

bazel run //tools:examples_raze
diff --git a/examples/remote/binary_dependencies/cargo/crates.bzl b/examples/remote/binary_dependencies/cargo/crates.bzl
index b4142103..f3f2ba9e 100644
--- a/examples/remote/binary_dependencies/cargo/crates.bzl
+++ b/examples/remote/binary_dependencies/cargo/crates.bzl
@@ -772,7 +772,7 @@ def remote_binary_dependencies_fetch_remote_crates():
     maybe(
         http_archive,
         name = "remote_binary_dependencies__texture_synthesis_cli__0_8_0",
-        url = "https://crates.io/api/v1/crates/texture-synthesis-cli/0.8.0/download",
+        url = "",
         type = "tar.gz",
         sha256 = "4d0a65298b735ba486081633e90469182d7c0ca59018fec1e81383bde852be2e",
         strip_prefix = "texture-synthesis-cli-0.8.0",

The url there should not be empty 😢

@illicitonion
Copy link
Collaborator Author

Oh no! Fix here: #442 (with a test!) - thanks for the heads up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support non-standard registries in remote mode
3 participants