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

treewide: String escape fixes #365186

Merged
merged 16 commits into from
Dec 15, 2024
Merged

treewide: String escape fixes #365186

merged 16 commits into from
Dec 15, 2024

Conversation

piegamesde
Copy link
Member

Follow-up to #350296 and #350774. Not only is the string indentation in Nix lang jank, but also the escaping rules. After this, there will still be some invalid escape sequences in Nixpkgs because of #365173.

Issues found by parsing Nixpkgs with https://gerrit.lix.systems/c/lix/+/2310/1

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I was surprised at how few rebuilds there were, but after seeing that many of these were in update scripts, that makes sense.

It's somewhat hilarious seeing the tests for a nasty string get the nasty string wrong.

I agree with the Lix issue that warning in these cases is the right thing to do.

@@ -37,7 +37,7 @@ let
"b"
]
];
nasty_string = "\"@\n\\\t^*\b\f\n\0\";'''$";
nasty_string = "\"@\n\\\t^*bf\n0\";'''$";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nasty indeed.

Copy link
Member Author

@piegamesde piegamesde Dec 14, 2024

Choose a reason for hiding this comment

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

This is especially hilarious because if you look at the expected output, it "correctly" contains b, f and 0. Which means that nobody bothered actually checking it?

I think it would be more interesting if that string actually contained the characters it intended to contain (except the NUL byte, because that horribly breaks Nix …), but I couldn't figure out how to easily put a line feed into a string without actually putting verbatim into the file, so …

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 15, 2024
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Dec 15, 2024
@philiptaron philiptaron merged commit 4d263e0 into NixOS:master Dec 15, 2024
40 checks passed
@piegamesde piegamesde added the backport release-24.11 Backport PR automatically label Dec 20, 2024
@nix-backports
Copy link

nix-backports bot commented Dec 20, 2024

Backport failed for release-24.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.11
git worktree add -d .worktree/backport-365186-to-release-24.11 origin/release-24.11
cd .worktree/backport-365186-to-release-24.11
git switch --create backport-365186-to-release-24.11
git cherry-pick -x 772ad025ecec551be8d09a26514092f5d7b9e740 ac27331127ab250e61442aae10db18aad642bd38 69f9bc37c77029490c81c8813da8c343ed917a9f 62fcc798988975fab297b87345d93919cd6f6389 50a57d5773aeedcca14e98a9d009955a0ce15914 8b344a79c972292d8b5c6426aa90df4bc06f4e9c dfc3657f0f81391b743213794e9f3b1775e1031a 0bf626239109203cd714e740c7bd3dfaa708d00a f9698477df7760f90f40d62a8185abf9da6e58f2 70e7ca4c3dd999edc78ab0ac0785f0eac2b54da3 bd2962950d2ba880bd1f02107ae02e61ecb3a256 6d815298c97fc3ccaef94a166dfa274736fcd0bc e044fed9a9023d457abce3880e7fe99d03be8b6a 160f865b6bc4be9056ff0a6fd853c466aee62c59 515a6a3386fe4b7b7dea97d72f41684d657c8235 591861f2b10f446efb5871d1467b245983b7e829

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package backport release-24.11 Backport PR automatically
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

4 participants