Skip to content

rustBuildCrate: properly handle cargo env pragmas with spaces#199300

Merged
aanderse merged 1 commit intoNixOS:masterfrom
Fuuzetsu:escape-rust-exports
Nov 16, 2022
Merged

rustBuildCrate: properly handle cargo env pragmas with spaces#199300
aanderse merged 1 commit intoNixOS:masterfrom
Fuuzetsu:escape-rust-exports

Conversation

@Fuuzetsu
Copy link
Member

@Fuuzetsu Fuuzetsu commented Nov 3, 2022 •
edited
Loading

There are two problems: first that we end up splitting on spaces in the loop. Even when that is fixed, we still would split on spaces in the export inside the loop. We need to guard against both.

Fixes #199298

Confirmed that it fixes the case mentioned in the ticket:

[nix-develop]$ $(nix-build -I nixpkgs=/home/shana/programming/nixpkgs Cargo.nix -A rootCrate.build  --no-out-link)/bin/nix-rustc-env-escape-repro
Expecting three words, got: first second third

I think this is going to cause a rebuild of every Rust package even if they were unaffected, not much we can do here.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Nov 3, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Nov 3, 2022
@Fuuzetsu
Copy link
Member Author

Fuuzetsu commented Nov 3, 2022

Ah, this breaks on multiple variables being present: I guess splitting on spaces is what made it work before. I'll fix the patch.

@Fuuzetsu
Copy link
Member Author

Fuuzetsu commented Nov 3, 2022

Ah, this breaks on multiple variables being present: I guess splitting on spaces is what made it work before. I'll fix the patch.

Aha, this patch actually prints empty line if nothing matches... Trying to figure out how to get it to work nicely while still producing escaped stuff...

@Fuuzetsu
rustBuildCrate: properly handle cargo env pragmas with spaces
233205c 
There are two problems: first that we end up splitting on spaces in the
loop. Even when that is fixed, we still would split on spaces in the
`export` inside the loop. We need to guard against both.

Fixes NixOS#199298

Confirmed that it fixes the case mentioned in the ticket:

```console
[nix-develop]$ $(nix-build -I nixpkgs=/home/shana/programming/nixpkgs Cargo.nix -A rootCrate.build  --no-out-link)/bin/nix-rustc-env-escape-repro
Expecting three words, got: first second third
```

I think this is going to cause a rebuild of every Rust package even if
they were unaffected, not much we can do here.
@Fuuzetsu Fuuzetsu force-pushed the escape-rust-exports branch from db620b7 to 233205c Compare November 3, 2022 05:49
@Fuuzetsu
Copy link
Member Author

Fuuzetsu commented Nov 3, 2022

OK, now I think it's ready. I made a derp with escaping myself so it didn't work if multiple lines were present. I addressed the issue.

@Fuuzetsu Fuuzetsu mentioned this pull request Nov 7, 2022
Merged
13 tasks
@lblasc
Copy link
Contributor

lblasc commented Nov 16, 2022

I've tested this patch for a week with crate2nix, no problems so far 👍

@aanderse can you merge this one ?

@lblasc lblasc requested a review from aanderse November 16, 2022 07:23
@Fuuzetsu
Copy link
Member Author

Fuuzetsu commented Nov 16, 2022

As another data point, we've been running it for 2 weeks on a medium(?) size workspace:

[nix-develop]$ cargo tree | wc -l
2958

No issues.

aanderse
aanderse approved these changes Nov 16, 2022
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks everyone!

@aanderse aanderse merged commit adbe2f5 into NixOS:master Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aanderse aanderse aanderse approved these changes

Assignees

No one assigned

Labels

6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rust: cargo and buildRustCrate disagree on escaping of rustc-env directives (and probably more)

3 participants

@Fuuzetsu @lblasc @aanderse