WIP: Decode url-encoded branch and tag names

[rossng]

Problem

The v4 Cargo lockfile format became the default fairly recently, and it includes a change which applies URL encoding to e.g. branch name parameters in Git URLs.

Cargo.lock v3

[[package]]
name = "ustr"
version = "0.9.0"
source = "git+https://github.com/bouk/ustr?branch=bouk/spinlock-note#7442ddf3a57ab89ac0a48556c76d25606b774eb7"
dependencies = [
 "ahash 0.7.8",
 "byteorder",
 "lazy_static",
 "serde",
 "spin 0.7.1",
]

Cargo.lock v4

[[package]]
name = "ustr"
version = "0.9.0"
source = "git+https://github.com/bouk/ustr?branch=bouk%2Fspinlock-note#7442ddf3a57ab89ac0a48556c76d25606b774eb7"
dependencies = [
 "ahash 0.7.8",
 "byteorder",
 "lazy_static",
 "serde",
 "spin 0.7.1",
]

naersk downloads these git dependencies to disk and then uses Cargo's source replacement feature to point the build at this directory so it can be built offline. However, the new lockfile format results in config.toml entries like this:

[source."https://github.com/bouk/ustr?branch=bouk%2Fspinlock-note"]
branch = "bouk%2Fspinlock-note"
git = "https://github.com/bouk/ustr"
replace-with = "git"

But these don't get picked up because the URL-encoded branch property no longer matches. Then at build time, Cargo itself attempts to download the crate from (e.g.) GitHub and gets blocked by the Nix sandbox, causing it to fail.

> warning: spurious network error (3 tries remaining): failed to resolve address for github.com: Temporary failure in name resolution; class=Net (12)
> warning: spurious network error (2 tries remaining): failed to resolve address for github.com: Temporary failure in name resolution; class=Net (12)
> warning: spurious network error (1 tries remaining): failed to resolve address for github.com: Temporary failure in name resolution; class=Net (12)
> error: failed to get `ustr` as a dependency of package `foo v0.1.0 (/build/dummy-src/foo)`
>
> Caused by:
>   failed to load source for dependency `ustr`
>
> Caused by:
>   Unable to update https://github.com/bouk/ustr?branch=bouk%2Fspinlock-note#7442ddf3
>
> Caused by:
>   failed to clone into: /build/dummy-src/.cargo-home/git/db/ustr-7b9d040a5bdb585c
>
> Caused by:
>   revision 7442ddf3a57ab89ac0a48556c76d25606b774eb7 not found
>
> Caused by:
>   network failure seems to have happened
>   if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
>   https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli
>
> Caused by:
>   failed to resolve address for github.com: Temporary failure in name resolution; class=Net (12)
> [naersk] cargo returned with exit code 101, exiting

Solution

Apply URL decoding to the branch and tag before writing out the config.toml file. I don't see how rev could contain an encoded character, but let me know if it should be done there too.

Todo

• Read the lockfile format version and do not apply URL decoding before v4
• Add a test for both v3 and v4 scenarios

[nmattia]

very cool, thanks @rossng ! I'll convert the PR to draft for now, just ping me when you need the review.

Alternatively: would it make sense to only support v4?

[chrisguida]

Thanks for writing this! I used this in my project today and it fixed the v3->v4 issue!

chrisguida/smaug#69

Notice CI failing beforehand: https://github.com/chrisguida/smaug/actions/runs/13045323154

But now it passes! https://github.com/chrisguida/smaug/actions/runs/13077563488

[rossng]

I tried writing a test for this today and it wasn't too easy. The existing test git-dep-by-branch-with-slash test is quite applicable here, but it uses a git+file:// URL and so actually doesn't seem to trigger the failure case.

It seems like the test needs to retrieve a git repository over the network to actually test the failure case. A couple of ideas:

• run the git daemon locally
  • I don't think this can happen inside the Nix build itself, the script/test would probably have to launch it and create a sample dependency to serve.
• point the test at a crate hosted on (e.g.) GitHub
  • easy, but introduces an external dependency to the test suite.

Would appreciate any input or ideas.

[nmattia]

@rossng sorry, forgot to circle back!

I tried writing a test for this today and it wasn't too easy.

Then don't worry about this too much. Seems like a good improvement so I'd suggest merging it as-is. If this part of the codebase ends up having to be modified heavily in the future again then we can reconsider.

[jraygauthier]

Any updates here? We just hit this branch url encoded issue and this appears likely to fix it.

[rossng]

My PR wasn't quite right, but I don't have time to work on it, unfortunately.

[jraygauthier]

Ok, any details as to what was missing?

[rossng]

I don't remember exactly, but I know I encountered something while trying to write the test for this behaviour. Like I was transforming the name at fundamentally the wrong stage of the process.

[nmattia]

I'll have a quick look today and see if I can unblock this; do you guys have 1-2 examples of recent manifests with the new encoding?

[jraygauthier]

I'll have a quick look today and see if I can unblock this; do you guys have 1-2 examples of recent manifests with the new encoding?

Nice, here's the git revision adding the problematic patch directive that started to fail our build:

https://bitbucket.org/amotus/fundamentum-edge-daemon/commits/5724cbc79fa4fb993d81419dc7960eb0d4e6ba45

As you can see below, it fails to build because of the "/" character in one of the the branch name (jou/file-transfer-feature) refered to by one of the patch directive:

$ nix build "git+https://bitbucket.org/amotus/fundamentum-edge-daemon.git?rev=5724cbc79fa4fb993d81419dc7960eb0d4e6ba45"#fundamentum-edge
error: builder for '/nix/store/p341d0x5ilcg94xcir0bv8xnm4apgwsb-fundamentum-edge-daemon-deps-1.8.0.drv' failed with exit code 101;
       last 25 log lines:
       > [naersk] RUSTFLAGS:
       > [naersk] CARGO_BUILD_RUSTFLAGS: -C linker=cc
       > [naersk] CARGO_BUILD_RUSTFLAGS (updated): -C linker=cc --remap-path-prefix /nix/store/ly3ym1a0www4ndmrhqp9l1b8cafqp3sx-crates-io-dependencies=/sources --remap-path-prefix /nix/store/wi8gwr3q0ycx13dcl7wb7a5b91q7gndx-git-dependencies=/sources
       > Running phase: buildPhase
       > cargo build $cargo_release -j "$NIX_BUILD_CORES" --message-format=$cargo_message_format
       >     Updating git repository `https://bitbucket.org/amotus/fundamentum-sdk-mqtt.git`
       > warning: spurious network error (3 tries remaining): failed to resolve address for bitbucket.org: Temporary failure in name resolution; class=Net (12)
       > warning: spurious network error (2 tries remaining): failed to resolve address for bitbucket.org: Temporary failure in name resolution; class=Net (12)
       > warning: spurious network error (1 tries remaining): failed to resolve address for bitbucket.org: Temporary failure in name resolution; class=Net (12)
       > error: failed to load source for dependency `fundamentum-sdk-mqtt`
       >
       > Caused by:
       >   Unable to update https://bitbucket.org/amotus/fundamentum-sdk-mqtt.git?branch=jou%2Ffile-transfer-feature#6e3c4935
       >
       > Caused by:
       >   failed to clone into: /build/dummy-src/.cargo-home/git/db/fundamentum-sdk-mqtt-ec156483963a0fbb
       >
       > Caused by:
       >   network failure seems to have happened
       >   if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
       >   https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli
       >
       > Caused by:
       >   failed to resolve address for bitbucket.org: Temporary failure in name resolution; class=Net (12)
       > [naersk] cargo returned with exit code 101, exiting
       For full logs, run:
         nix log /nix/store/p341d0x5ilcg94xcir0bv8xnm4apgwsb-fundamentum-edge-daemon-deps-1.8.0.drv
error: 1 dependencies of derivation '/nix/store/q8xr7fjs5w2mdyb8g5n5vd213f3my991-fundamentum-edge-daemon-1.8.0.drv' failed to build

Our current workaround is to pin the problematic entry to a rev (or alternatively not to use the '/' in the branch name):

https://bitbucket.org/amotus/fundamentum-edge-daemon/commits/596c671d279300733c191ce2e5566942682c95ed

Which now builds fine with this workaround applied:

$ nix build "git+https://bitbucket.org/amotus/fundamentum-edge-daemon.git?rev=596c671d279300733c191ce2e5566942682c95ed"#fundamentum-edge
# -> Success

[nmattia]

thanks! I think it should be a fairly quick fix but I have to do some overdue maintenance first; hoping to get to this on Monday!

[nmattia]

Took me a bit longer than expected to get to this!

I've included @rossng 's fix in here and added a test. @jraygauthier can you try it out?