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

gcc: install info files serially #229898

Merged
merged 1 commit into from
May 10, 2023

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 label 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.

@raboof
Copy link
Member Author

raboof commented May 9, 2023

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.

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

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
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 reputable 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
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 :(

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.

gcc: undeterministic info pages
5 participants