Skip to content

boehmgc: explain+refine test-disablement on darwin#199980

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

boehmgc: explain+refine test-disablement on darwin#199980
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Nov 7, 2022

Description of changes

Commit 172f9cf disabled the checks for boehmgc on darwin-x86_64 with the comment "gctest fails under emulation on aarch64-darwin".

Presumably this is because the test does not work under Apple's Rosetta binary translator, which is used to execute x86_64 binaries on aarch64 hosts. The check phase would only use this translator if a narrower condition is met:

stdenv.isDarwin && stdenv.buildPlatform.isAarch64 && stdenv.hostPlatform.isx86_64

Let's use that condition instead, mainly as documentation so people don't think that the boehm-gc test suite is unreliable (this happened already in #198591). The boehm-gc test suite is actually quite good, and it is extremely important to run the tests for this particular package because of the number of advanced conservative-collector techniques it uses. Malfunctions here can cause use-after-free() bugs that don't correspond to any particular invocation of free() the way they would in a C/C++ program. They are incredibly difficult to track down.

CC: @zowoq @vcunat

#191339 (comment)

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.

Commit 172f9cf disabled the checks
for boehmgc on darwin-x86_64 with the comment "`gctest` fails under
emulation on aarch64-darwin".

Presumably this is because the test does not work under Apple's
Rosetta binary translator, which is used to execute x86_64 binaries
on aarch64 hosts.  The `check` phase would only use this translator
if a narrower condition is met:

```
stdenv.isDarwin && stdenv.buildPlatform.isAarch64 && stdenv.hostPlatform.isx86_64
```

Let's use that condition instead, mainly as documentation so people
don't think that the boehm-gc test suite is unreliable (this
happened already in #198591).
The boehm-gc test suite is actually quite good, and it is extremely
important to run the tests for this particular package because of
the number of advanced conservative-collector techniques it uses.
Malfunctions here can cause use-after-`free()` bugs that don't
correspond to any particular invocation of `free()` the way they
would in a C/C++ program.  They are incredibly difficult to track
down.
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Nov 7, 2022
@ghost ghost marked this pull request as draft November 7, 2022 06:29
@ghost ghost marked this pull request as ready for review November 7, 2022 06:30
@ofborg ofborg bot requested a review from AndersonTorres November 7, 2022 06:40
@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: 0 This PR does not cause any packages to rebuild on Linux. labels Nov 7, 2022
@vcunat
Copy link
Copy Markdown
Member

vcunat commented Nov 7, 2022

No, that won't work. The rosetta builds don't trigger stdenv.buildPlatform.isAarch64. It's not normal cross-compilation; for that we wouldn't really need rosetta (just for tests perhaps). It's similar to how we build i686-linux builds on x86_64-linux machines – there you also don't have different buildPlatform.

@vcunat vcunat closed this Nov 7, 2022
@vcunat
Copy link
Copy Markdown
Member

vcunat commented Nov 7, 2022

If you want a narrower approach, disabling just the single test might be relatively easy.

@ghost
Copy link
Copy Markdown
Author

ghost commented Nov 7, 2022

@vcunat I completely forgot that, as staging captain, your pre-release crunch period starts a month earlier. This can absolutely wait until after 22.11.

PS, thanks for explaining how Rosetta is used in nixpkgs; that totally makes sense.

@ghost ghost deleted the pr/boehmgc/explain branch November 7, 2022 21:37
@vcunat
Copy link
Copy Markdown
Member

vcunat commented Nov 7, 2022

It's not really in nixpkgs... but on Hydra, as x86 Apple HW would be less cost-effective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: darwin Running or building packages on Darwin 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: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant