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

Usage of sparse registries not working #1854

Closed
FaBrand opened this issue Mar 3, 2023 · 19 comments · Fixed by #1857
Closed

Usage of sparse registries not working #1854

FaBrand opened this issue Mar 3, 2023 · 19 comments · Fixed by #1857

Comments

@FaBrand
Copy link
Contributor

FaBrand commented Mar 3, 2023

https://blog.rust-lang.org/2022/06/22/sparse-registry-testing.html introduced usage of a sparse registry.
The feature is stablized and will become available in the stable channel in a few days:

Sparse registries are now stabilized and will be available in Rust 1.68, which will be released on 2023-03-09

crate-index is used here to parse the config that would hold the configuration:

[registries.artifactory]
index = "sparse+https://artifactory.company.my/artifactory/api/cargo/external-index-crates-io-remote/index/"
[source.artifactory]
registry = "sparse+https://artifactory.company.my/artifactory/api/cargo/external-index-crates-io-remote/index/"

However this fails to parse with:

Error: Failed to locate crate indexes

Caused by:
    0: Failed to load index for url: sparse+https://artifactory.company.my/artifactory/api/cargo/external-index-crates-io-remote/index/
    1: 'sparse+https://artifactory.company.my/artifactory/api/cargo/external-index-crates-io-remote/index/' is not a valid registry url

There is already an issue i could find since the feature is not yet supported by rust-crates-index:
frewsxcv/rust-crates-index#87

@illicitonion
Copy link
Collaborator

I have a draft PR locally to support this, but need to wait on some internal approvals before I can publish it - will try to get this out soon

@illicitonion
Copy link
Collaborator

See #1857 - please give it a go and see if it works for you!

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 7, 2023

@illicitonion Thanks for the quick feedback. I'll check it now. Need to reproduce the github actions first to build the release

@illicitonion
Copy link
Collaborator

See https://github.com/bazelbuild/rules_rust/blob/main/crate_universe/DEVELOPMENT.md for instructions for how to use a custom cargo-bazel binary.

(You'll need to be configuring your WORKSPACE to use 1.68.0 or newer to use the feature)

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 7, 2023

See https://github.com/bazelbuild/rules_rust/blob/main/crate_universe/DEVELOPMENT.md for instructions for how to use a custom cargo-bazel binary.

(You'll need to be configuring your WORKSPACE to use 1.68.0 or newer to use the feature)

Thanks for the pointers 👍

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 7, 2023

Ok, managed to build and test it. However it fails with another error.
(Redacted internal domains) But the structure of the url is dentical

STDOUT ------------------------------------------------------------------------

STDERR ------------------------------------------------------------------------
Error: Failed to locate crate indexes

Caused by:
    0: Failed to load index for url: sparse+https://my.artifactory.sub.company.net/artifactory/api/cargo/external-index-crates-io-remote/index/
    1: unsupported URL protocol; class=Net (12)
    2: unsupported URL protocol; class=Net (12)
[net]
git-fetch-with-cli = true

[registry]
default = "artifactory"

[source.crates-io]
replace-with = "artifactory"


[registries.artifactory]
index = "sparse+https://my.artifactory.sub.company.net/artifactory/api/cargo/external-index-crates-io-remote/index/"
token = ""

[source.artifactory]
registry = "sparse+https://my.artifactory.sub.company.net/artifactory/api/cargo/external-index-crates-io-remote/index/"

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 7, 2023

Using rules_rust at HEAD of your branch:

$ git rev-parse HEAD
72779de271d3759e1cdaeebcfb90b3d8292bcc90

I checked that the newly built cargo-bazel is used:

sha256sum ~/.bazel/rust_ob/external/crate_index/cargo-bazel ~/rules_rust/crate_universe/target/debug/cargo-bazel
95829bf689335cf05b021ffbc06dbb07fb03e182ffa7190eb1e2d95e68fc68b8  ~/.bazel/rust_ob/external/crate_index/cargo-bazel
95829bf689335cf05b021ffbc06dbb07fb03e182ffa7190eb1e2d95e68fc68b8  ~/rules_rust/crate_universe/target/debug/cargo-bazel

And the rust toolchain version that i configured in my WORKSPACE

$ ~/.bazel/rust_ob/external/rust_linux_x86_64__x86_64-unknown-linux-gnu__beta_tools/bin/cargo --version
cargo 1.68.0-beta.6 (115f34552 2023-02-26)

I merged your fork-branch with bazelbuild/rules_rust/main@d9ecc8df4c244ec6e82cd2d80ef45d6b46132ee7, and received the same error.

Also when using rules_rust with release 0.18.0 as well as HEAD merged and unmerged with main, same error

@illicitonion
Copy link
Collaborator

Also when using rules_rust with release 0.18.0 as well as HEAD merged and unmerged with main, same error

Just to check, is this saying 0.18.0 without this patch is broken for you, or just 0.18.0 with this patch?

@illicitonion
Copy link
Collaborator

In terms of debugging the issue, I would expect this block to be getting hit for your index: https://github.com/bazelbuild/rules_rust/pull/1857/files#diff-8e0f53a4c6423a010464dacf0b46519b5e50994ae4a2bafc8c1dd0898532609eR303-R305 (because url.starts_with("sparse+https://")), but the error you're pasting comes from the else block suggesting that code isn't getting hit (https://github.com/bazelbuild/rules_rust/pull/1857/files#diff-8e0f53a4c6423a010464dacf0b46519b5e50994ae4a2bafc8c1dd0898532609eR328)

If you could do some debugging around the url value to work out why else if url.starts_with("sparse+https://") isn't getting selected, that'd be really useful...

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 7, 2023

Also when using rules_rust with release 0.18.0 as well as HEAD merged and unmerged with main, same error

Just to check, is this saying 0.18.0 without this patch is broken for you, or just 0.18.0 with this patch?

I ran this to test the execution (Workspace bazelrc only contains the beta channel and output base config hence the --nohome_rc)

export CARGO_BAZEL_GENERATOR_URL=file:///home/user/rules_rust/crate_universe/target/debug/cargo-bazel
CARGO_BAZEL_REPIN=true bazel --nohome_rc sync --only=crate_index

I tested with the released @rules_rust at version 0.18.0 and a local_repository that points to my working copy of rules rust.

tldr;
Different versions of @rules_rust with the same debug build of cargo-bazel yield the same error.

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 7, 2023

In terms of debugging the issue, I would expect this block to be getting hit for your index: https://github.com/bazelbuild/rules_rust/pull/1857/files#diff-8e0f53a4c6423a010464dacf0b46519b5e50994ae4a2bafc8c1dd0898532609eR303-R305 (because url.starts_with("sparse+https://")), but the error you're pasting comes from the else block suggesting that code isn't getting hit (https://github.com/bazelbuild/rules_rust/pull/1857/files#diff-8e0f53a4c6423a010464dacf0b46519b5e50994ae4a2bafc8c1dd0898532609eR328)

If you could do some debugging around the url value to work out why else if url.starts_with("sparse+https://") isn't getting selected, that'd be really useful...

Hm, true...
I'll check and report back

@illicitonion
Copy link
Collaborator

(Also, I don't actually have a custom sparse index to test against, so I've only tested the first branch there, the "override crates.io to use sparse", so... You're running into some completely untested code, sorry!)

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 7, 2023

Had a bit of trouble with debugging so far. For some reason the LockGenerator was complaining about the --locked
I set the working directory, env vars and all arguments the same as bazel would but still received this error:

error: the lock file /tmp/.tmpwBMe9I/Cargo.lock needs to be updated but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network, remove the --locked flag and use --offline instead.

Error: Failed to fetch crates for lockfile: exit status: 101

Could not get around this without 'building' with bazel so far.
I managed to attach a debugger to the process invoked via bazel.

Regarding the url.starts_with;

i saw in the debugger that the index_urls (https://github.com/illicitonion/rules_rust/blob/sparse-index/crate_universe/src/splicing.rs#L301) only has one entry.
And that is "https://github.com/rust-lang/crates.io-index"

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 7, 2023

I checked the generated lock file in the temporary manifest_dir. And it contains the github.com/rust-lang/crates.io-index urls.
I checked the state of the file right after the LockfileGenerator ran.

source = "registry+https://github.com/rust-lang/crates.io-index"

Compared to a regular cargo build, the lockfile contains the correct replaced source:

source = "sparse+https://my.artifactory.sub.company.net/artifactory/api/cargo/external-index-crates-io-remote/index/"

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 7, 2023

I looked a bit futher. I tracked the creation of the initial Cargo.toml and the subsequent calls to cargo.

The original Cargo.toml contains the dependencies as declared in the Workspace.

I did the same with cargo directly. And it generates a registry key for the dependency added.
Not sure why it needs to add it since i set my default registry to another one.
(Very new to the rust ecosystem, so maybe a lack of understanding on my part)

If i then modify the Cargo.toml in the tmp dir with adding the registry to the Cargo.toml (cargo-bazel stopped in debugger before the cargo update call is issued) and run cargo update manually, it generates the lock file as expected.

I suppose the error with the url is just a followup one.

When doing this manual modification the Lockfile fails to be loaded with:

STDERR ------------------------------------------------------------------------
Error: Failed to load lockfile: /tmp/.tmpDrR87q/Cargo.lock

Caused by:
    parse error: parse error: unsupported source protocol: sparse for key `package.source` at line 325 column 1

@illicitonion
Copy link
Collaborator

I just re-pushed #1857 after testing it out with a cargo config file which looks a lot like yours - can you give it another go @FaBrand? Thanks!

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 13, 2023

I gave it another go @illicitonion,
from what i can tell so far: it works! (Checked with 0e5686f)

I still have some 40x issues with our artifactory it seems, but the generated defs.bzl in my crates_repository(..) contains seemingly correct urls to the registry i have configured in my .cargo/config.toml it receives.
Next round of debugging is going to be an internal one :D

Thanks!!

@FaBrand
Copy link
Contributor Author

FaBrand commented Mar 16, 2023

@illicitonion Just wanted to followup after i solved my artifactory access issues and after the 0.19.0 release was cut.
Everything works great and hermetic as expected with the sparse registry configuration for artifactory :)
Turns out we also had configuration errors in our AF Repo. But those got resolved and works like a charm :)

My testing setup:

#!/bin/bash
set -euxo pipefail

# Should fail to check that no connection to outside can be made
curl -XGET -L https://index.crates.io || true
curl -XGET -L https://crates.io || true
curl -XGET -L https://github.com/ || true

echo '' > Cargo.lock
echo '' > Cargo.Bazel.lock

bazel shutdown

bazel clean --expunge

CARGO_BAZEL_REPIN=true bazel sync --only=crate_index

bazel run //parse_command_lines

My Cargo config (also setup along https://www.jfrog.com/confluence/display/JFROG/Cargo+Package+Registry#CargoPackageRegistry-SettingUpCargoIndexingUsingSparseIndexing)

[net]
git-fetch-with-cli = true

# Makes artifactory the default registry and saves passing --registry parameter
[registry]
default = "artifactory"

[source.crates-io]
replace-with = "artifactory"


[registries.artifactory]
index = "sparse+https://my.artifactory.instance.net/artifactory/api/cargo/external-index-crates-io-remote/index/"
token = ""

[source.artifactory]
registry = "sparse+https://my.artifactory.instance.net/artifactory/api/cargo/external-index-crates-io-remote/index/"

@illicitonion
Copy link
Collaborator

Hurrah, I'm glad! Thanks for circling back!

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 a pull request may close this issue.

2 participants