Skip to content

gcc: install info files serially#229898

Merged
vcunat merged 1 commit intoNixOS:stagingfrom
raboof:gcc-install-info-pages-serially
May 10, 2023
Merged

gcc: install info files serially#229898
vcunat merged 1 commit intoNixOS:stagingfrom
raboof:gcc-install-info-pages-serially

Conversation

@raboof
Copy link
Member

@raboof raboof commented May 4, 2023

Description of changes

installing info files in parallel is dangerous, because install-info will write to a dir-file as a side-effect, and it has no protection against multiple install-info processes running in parallel and overwriting each others' changes.

Local fix until we can fix the Makefile.in generation upstream

Fixes #229470

Things done

Tested the concept locally with the following Makefile:

all: install-info

.NOTPARALLEL: install-info

install-info:: 1.to 2.to 3.to 4.to

%.to: %.from
	echo "converting $^"
	sleep 10
	echo "converted $^"

clean:
	rm *.to
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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.

@raboof raboof requested a review from matthewbauer as a code owner May 4, 2023 13:00
@ofborg ofborg bot added the 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. label May 4, 2023
@ofborg ofborg bot requested review from Ericson2314, Synthetica9 and vcunat May 4, 2023 15:32
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels May 4, 2023
@raboof raboof changed the base branch from master to staging May 4, 2023 15:33
@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label May 4, 2023
@trofi
Copy link
Contributor

trofi commented May 4, 2023

The workaround looks good. We will probably want to apply it to all gcc versions.

@vcunat vcunat added the 6.topic: reproducible builds Run nix-build twice and get the same result. label May 5, 2023
@raboof
Copy link
Member Author

raboof commented May 9, 2023 •
edited
Loading

We will probably want to apply it to all gcc versions.

Are you sure we want this? I'd like to follow upstream as much as possible, and as for 12 we have a particular motivation to want it to be reproducible (e.g. we use this to build our installation media), it seems most conservative not to also apply it to the older versions?

@trofi
Copy link
Contributor

trofi commented May 9, 2023

We will probably want to apply it to all gcc versions.

Are you sure we want this? ... and as for 12 we have a particular motivation to want it to be reproducible (e.g. we use this to build our installation media), it seems most conservative not to also apply it to the older versions?

Out of curiosity who do you mean by "we"? I would normally expect all the nixpkgs packages should be determinitstic. The change is very safe and well-contained. If you are not comfortable of doing it I can also it to over gcc versions as a separate PR.

I'd like to follow upstream as much as possible

gcc is an example of very mangled packages in nixpkgs compared to what upstream suggests doing. I would say this patch does not make anything worse.

@raboof
gcc: install info files serially
f3995ce 
installing info files in parallel is dangerous, because
`install-info` will write to a `dir-file` as a side-effect,
and it has no protection against multiple `install-info`
processes running in parallel and overwriting each others'
changes.

Local fix until we can fix the `Makefile.in` generation
upstream

Fixes NixOS#229470
@raboof raboof force-pushed the gcc-install-info-pages-serially branch from 28d07d8 to f3995ce Compare May 9, 2023 17:03
@raboof
Copy link
Member Author

raboof commented May 9, 2023 •
edited
Loading

Out of curiosity who do you mean by "we"?

'NixOS', which is admittedly an ill-defined 'we' :)

I would normally expect all the nixpkgs packages should be determinitstic.

True, I was thinking "let's focus on the currently-widely-used things and not worry about older versions too much"

The change is very safe and well-contained. gcc is an example of very mangled packages in nixpkgs compared to what upstream suggests doing. I would say this patch does not make anything worse.

OK, I'll give it a go

@raboof raboof marked this pull request as draft May 9, 2023 17:07
@raboof
Copy link
Member Author

raboof commented May 9, 2023

OK, I'll give it a go

That applied surprisingly painlessly!

@raboof raboof marked this pull request as ready for review May 9, 2023 19:50
trofi
trofi approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@trofi trofi left a comment

Choose a reason for hiding this comment

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

LGTM. All the $ nix build --no-link -f. gcc48 gcc49 gcc6 gcc7 gcc8 gcc9 gcc10 gcc11 gcc12 succeeded.

@mweinelt mweinelt added the 12.approvals: 1 This PR was reviewed and approved by one person. label May 9, 2023
@ofborg ofborg bot requested a review from veprbl May 9, 2023 21:36
@vcunat vcunat merged commit ae3f6c9 into NixOS:staging May 10, 2023
@cole-h cole-h removed the ofborg-internal-error Ofborg encountered an error label May 18, 2023
@ghost
Copy link

ghost commented Jun 13, 2023

Hi, this is causing the entire gcc build to use only a single core.

According to the GNU Make manual, 5.4.1 Disabling Parallel Execution the patch added by this PR marks install-info and all of its prerequisites as needing to be built serially. I really don't think that's what we want to do.

@ghost ghost mentioned this pull request Jun 13, 2023
Merged
12 tasks
@raboof
Copy link
Member Author

raboof commented Jun 13, 2023

Hi, this is causing the entire gcc build to use only a single core.

According to the GNU Make manual, 5.4.1 Disabling Parallel Execution the patch added by this PR marks install-info and all of its prerequisites as needing to be built serially. I really don't think that's what we want to do.

Ouch, that indeed was not the intent - I had read that documentation, but assumed it would only run the direct prerequisite builds in parallel, not everything transitively. I thought I had checked that but perhaps not sufficiently/correctly. Will look into this (unless someone beats me to it)

@trofi
Copy link
Contributor

trofi commented Jun 13, 2023

make-4.4 is expected to behave like that. The problem is that gcc in nixpkgs does not use gnumake from nixpkgs:

$ nix develop -f. gcc.cc
$ make --version
GNU Make 4.2.1

@trofi
Copy link
Contributor

trofi commented Jun 13, 2023

We discovered the same in https://gcc.gnu.org/PR109898#c4

@trofi
Copy link
Contributor

trofi commented Jun 13, 2023

enableParallelInstalling = false; is probably the best non-invasive alternative we have compared to serial build :(

@raboof raboof mentioned this pull request Jun 13, 2023
Merged
12 tasks
@trofi trofi mentioned this pull request Sep 6, 2023
Closed
9ary added a commit to devkitNoob/devkitNoob that referenced this pull request Oct 12, 2025
@9ary @raboof
toolchains: install info files serially
4be2ce2 
installing info files in parallel is dangerous, because
`install-info` will write to a `dir-file` as a side-effect,
and it has no protection against multiple `install-info`
processes running in parallel and overwriting each others'
changes.

See: https://gcc.gnu.org/PR109898
See: NixOS/nixpkgs#229898
Co-authored-by: Arnout Engelen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@Synthetica9 Synthetica9 Awaiting requested review from Synthetica9

@vcunat vcunat Awaiting requested review from vcunat

@veprbl veprbl Awaiting requested review from veprbl

1 more reviewer

@trofi trofi trofi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: reproducible builds Run nix-build twice and get the same result. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

gcc: undeterministic info pages

5 participants

@raboof @trofi @mweinelt @vcunat @cole-h