Skip to content

cc-wrapper: disable response files for ccache#178280

Merged
kira-bruneau merged 1 commit intoNixOS:masterfrom
veprbl:pr/ccache_clang_fix
Apr 15, 2023
Merged

cc-wrapper: disable response files for ccache#178280
kira-bruneau merged 1 commit intoNixOS:masterfrom
veprbl:pr/ccache_clang_fix

Conversation

@veprbl
Copy link
Copy Markdown
Member

@veprbl veprbl commented Jun 19, 2022 •
edited
Loading

The problem is that the ccache wrapper does not work with response
files (it seems like the fd doesn't survive after exec in the
makeWrapper wrapper).

with import ./. {};
  
let
  _ccacheStdenv = ccacheStdenv.override {
    extraConfig = ''
      export CCACHE_DIR=/tmp/ccache
      export CCACHE_UMASK=007
    '';
  };
in
  hello.override { stdenv = _ccacheStdenv; }
Description of changes

Resolves: #119779

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.

@veprbl
cc-wrapper: disable response files for ccache
daa77ca 
The problem is that the ccache wrapper does not work with response
files (it seems like the fd doesn't survive after exec in the
makeWrapper wrapper).
@veprbl veprbl requested a review from Ericson2314 as a code owner June 19, 2022 21:41
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jun 19, 2022
@nixos-discourse
Copy link
Copy Markdown

nixos-discourse commented Aug 12, 2022

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1089

Atemu
Atemu approved these changes Apr 15, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Diff LGTM and this fixes the issue.

I'm inclined to merge this.

@Atemu Atemu requested review from kira-bruneau and r-burns April 15, 2023 10:29
@Atemu
Copy link
Copy Markdown
Member

Atemu commented Apr 15, 2023

Pinged ccache maintainers.

kira-bruneau
kira-bruneau approved these changes Apr 15, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@kira-bruneau kira-bruneau left a comment

Choose a reason for hiding this comment

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

Oh thanks for the ping @Atemu! This LGTM!!

@kira-bruneau kira-bruneau merged commit 99a9508 into NixOS:master Apr 15, 2023
@kira-bruneau
Copy link
Copy Markdown
Contributor

kira-bruneau commented Apr 15, 2023 •
edited
Loading

Oh wait I just noticed that it looks like ccache fixed parsing response files in v4.6.1: https://ccache.dev/releasenotes.html#_build_improvements_6 - this probably isn't needed anymore.

@Atemu
Copy link
Copy Markdown
Member

Atemu commented Apr 15, 2023

It wasn't working for me and the ccache version is 4.8 in my local tree I cherry-picked this onto.

@kira-bruneau
Copy link
Copy Markdown
Contributor

kira-bruneau commented Apr 15, 2023 •
edited
Loading

Ok, thanks! Just wanted to check first if ccache added supported for it in the meantime.

@Atemu
Copy link
Copy Markdown
Member

Atemu commented Apr 15, 2023

@kira-bruneau Btw, where did you see clang response files being supported in that changelog?

@kira-bruneau
Copy link
Copy Markdown
Contributor

kira-bruneau commented Apr 15, 2023

Oh it was Fixed parsing of MSVC response files. - but I'm assuming that would be different than the clang response files.

@reckenrode reckenrode mentioned this pull request Jun 10, 2023
Closed
@Artturin Artturin mentioned this pull request Jul 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kira-bruneau kira-bruneau kira-bruneau approved these changes

@Atemu Atemu Atemu approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@r-burns r-burns Awaiting requested review from r-burns

Assignees

No one assigned

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clang response files break ccache

4 participants

@veprbl @nixos-discourse @Atemu @kira-bruneau