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

cc-wrapper: convert cc-wrapper.sh to cc-wrapper.nix #266838

Closed
wants to merge 2 commits into from
Closed

cc-wrapper: convert cc-wrapper.sh to cc-wrapper.nix #266838

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 11, 2023

Description of changes

This commit converts cc-wrapper.sh into a .nix file consisting of one large string. This will allow future commits to hoist conditionals out of bash code, into Nix code. This allows to add or remove conditional code from the cc-wrapper without causing rebuilds on platforms where the condition is false.

Although this commit does not change the contents of the resulting /nix/store/*-cc-wrapper.sh it does unfortunately change the hash of that outpath, because Nix has way too many different hashing schemes. If you write ./cc-wrapper.sh you get one hashing scheme; if you write ''${builtins.readFile ./cc-wrapper.sh}'' you get a different one :(

Also enables

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/)
  • 23.11 Release Notes (or backporting 23.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.

This commit converts cc-wrapper.sh into a .nix file consisting of
one large string.  This will allow future commits to hoist
conditionals out of bash code, into Nix code.  This allows to add or
remove conditional code from the cc-wrapper **without causing
rebuilds on platforms where the condition is false**.
@ghost ghost marked this pull request as draft December 1, 2023 01:47
@ghost ghost marked this pull request as ready for review December 1, 2023 01:48
@ghost ghost requested a review from malt3 December 1, 2023 01:55
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Dec 1, 2023
@ghost
Copy link
Author

ghost commented Dec 1, 2023

@ofborg build stdenv

^ mainly for darwin

@ghost
Copy link
Author

ghost commented Dec 2, 2023

error: building of '/nix/store/a1zknb4ma2gawis0xkik2lgaf71phzdq-llvm-16.0.6.drv!dev' from .drv file timed out after 3600 seconds

Let's try that again

@ofborg build stdenv

Copy link
Contributor

@malt3 malt3 left a comment

Choose a reason for hiding this comment

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

This looks good to me. But I have to admit that I'm probably not experienced enough to review this change, since it could have far reaching effects. So I would strongly advise for more experienced reviewers to have a look as well!

@ghost ghost closed this Jan 23, 2024
@ghost ghost deleted the pr/cc-wrapper/nixify branch January 23, 2024 06:47
@malt3
Copy link
Contributor

malt3 commented Jan 23, 2024

Is the close intentional? Is there anything I can do to help you out with this?

@katexochen
Copy link
Contributor

@malt3 I think this PR was closed in in the light of #50105 (comment). While not necessary, Adam chose to closed some of their open PRs (which is of cause okay if they like to do so). If you want to continue this work you could cherry-pick the changes and open a new PR.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants