Fix (+) In vendored crates semver metadata

[ericmcbride]

• Resolves Vendoring crates with a "+" in the version breaks the deps graph #2524
• Revendored examples deps.
• Semver Metadata can break vendored dependencies, by cargo vendoring (foo-1.2.3+1.2.3) for example. The bazel labels will look for foo-1.2.3-1.2.3 which does not work.
• Root of the issue is, rules rust uses cargo to vendor the dependencies, which takes the semver + metadata, and generates the file path based off that (hence the + for the dependency in the issue)
• Tested by taking that dependency and make sure it loaded using the vendor_local_manifests example
• Did not include that dependency, because it would introduce a sys dep in CI
• Opted to do it within the scope of the vendor cli. When trying to change the version in the render_build_files call, it fixes the version in terms of the outputs and files declared in the vendor cli file, but then we do not have a handle to the generated vendor files from cargo, meaning we cant move the contents over near as easily. I also didn't want to break anything downstream that may rely on that info.

[ericmcbride]

Are worried about someones path on their computer having a + sign in it? If so i may need to not re use the sanitize function

[UebelAndre]

Why not have this logic happen in the place where we decide the output name?
https://github.com/bazelbuild/rules_rust/blob/0.41.0/crate_universe/src/rendering.rs#L286

That way we don't need to bother with an additional file op and this code function would be unchanged, right?

[ericmcbride]

I think I tried this and it had side effects somewhere . Let me change this line and make sure. Im all for smaller code changes.

[ericmcbride]

If you just change the label, it will put the build file in the correct path, but the Crate itself will be stored in the + file path, due to how cargo vendors the dep. I tried this my first attempt, and thats how I ended up with my current solution. We use cargo to run the vendor command, and it will auto generate the semver metadata with the plus sign

libbpf-sys-1.3.0+v1.3.0 
libbpf-sys-1.3.0-v1.3.0

>  ls libbpf-sys-1.3.0+v1.3.0 
bindings.h  build.rs  Cargo.toml  elfutils  libbpf  LICENSE  README.md  rebuild.sh  src  tests  zlib

>  ls libbpf-sys-1.3.0-v1.3.0 
BUILD.bazel

[UebelAndre]

What if you did the rename here?

https://github.com/bazelbuild/rules_rust/blob/0.41.0/crate_universe/src/rendering.rs#L292

If all this new code is doing is renaming a file, it seems like we could update the code to have a correct name in the first place. If the label shouldn't change then perhaps we can update just the output file? But the thing I'd like to avoid is renaming a file. We should be producing the correct data before writing.

[ericmcbride]

I agree with you 100 percent, but changing those labels only change the files for the BUILD files to be generated in.

https://github.com/bazelbuild/rules_rust/blob/0.41.1/crate_universe/src/metadata.rs#L423
This line of code the rules are using Cargo to generate versioned directories. This is where the libbpf-sys-1.3.0+v1.3.0 is coming from, not the rules or rendering themselves.

https://doc.rust-lang.org/cargo/commands/cargo-vendor.html

I dont see any options in there to allow the changing of the names. Unless im missing something here, the Cargo.lock file and cargo command is the source of truth here when generating the vendored, versioned directories and thats why I had to add that code.

[UebelAndre]

AAAAH! I see now!! Thank you for helping me and for being so patient!

In this case, could you make a new function that does the renames? I'd totally forgotten cargo is what writes the directory names. I think it would be much clearer to future readers if there was something that consumed the outputs value from the render, and then looked to rename existing directories as well as mutate the target output path and we hand that off to write_outputs just as we do today. That rename function could have a nice and noticeable docstring that can communicate what's going on.

Hopefully this isn't too much of a detour but if I know myself I'll forget about this in a month and be just as confused for similar changes 😅

[ericmcbride]

Awesome, I will get on it! And no problem. Took me a while to grok it too. I went down the exact same path as you so I know exactly the thought pattern you were going down 😅

[UebelAndre]

Thanks! Could you also add a unit test for this? 🙏

[UebelAndre]

Thank you! I had just one question

[UebelAndre]

Why not have this logic happen in the place where we decide the output name?
https://github.com/bazelbuild/rules_rust/blob/0.41.0/crate_universe/src/rendering.rs#L286

That way we don't need to bother with an additional file op and this code function would be unchanged, right?

[UebelAndre]

AAAAH! I see now!! Thank you for helping me and for being so patient!

In this case, could you make a new function that does the renames? I'd totally forgotten cargo is what writes the directory names. I think it would be much clearer to future readers if there was something that consumed the outputs value from the render, and then looked to rename existing directories as well as mutate the target output path and we hand that off to write_outputs just as we do today. That rename function could have a nice and noticeable docstring that can communicate what's going on.

Hopefully this isn't too much of a detour but if I know myself I'll forget about this in a month and be just as confused for similar changes 😅

[ericmcbride]

I moved the re mapping of the outputs from write_outputs into this function, that way the iteration still only happens one time. So it combines

    let outputs: BTreeMap<PathBuf, String> = outputs
        .into_iter()
        .map(|(path, content)| (out_dir.join(path), content))
        .collect();

With the renaming of the files if needed.

if nothing needs to be renamed, it simply just does the out_dir join, previously in the write_outputs function

[ericmcbride]

Example of SemVer change

[UebelAndre]

Awesome! Thank you so much!