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

pkgsCross.mingwW64.windows.mcfgthreads: add gcc13 compatible version #236598

Merged
merged 1 commit into from
Jun 11, 2023

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Jun 8, 2023

Upstream gcc-13 merged mcfgthreads support with a caveat: it's headers interface is not compatible with the patch nixpkgs was carrying in gcc-12 and before.

To keep both new (gcc13) and old (_pre_gcc13) version I held back previous windows.mcfgthreads attribute as
windows.mcfgthreads_pre_gcc_13. It is used for gcc before 13.

The change fixes the build of pkgsCross.mingwW64.stdenv itself and example program:

$ nix build --impure --expr 'with import ./. {}; pkgsCross.mingwW64.re2c.override { stdenv = pkgsCross.mingwW64.gcc11Stdenv; }'
Description of changes
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/)
  • 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.

Upstream `gcc-13` merged `mcfgthreads` support with a caveat: it's
headers interface is not compatible with the patch `nixpkgs` was
carrying in `gcc-12` and before.

To keep both new (`gcc13`) and old (`_pre_gcc13`) version I held back
previous `windows.mcfgthreads` attribute as
`windows.mcfgthreads_pre_gcc_13`. It is used for `gcc` before 13.

The change fixes the build of `pkgsCross.mingwW64.stdenv` itself and
example program:

    $ nix build --impure --expr 'with import ./. {}; pkgsCross.mingwW64.re2c.override { stdenv = pkgsCross.mingwW64.gcc11Stdenv; }'
@trofi trofi mentioned this pull request Jun 8, 2023
12 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jun 8, 2023
@trofi trofi merged commit b3ab40b into NixOS:master Jun 11, 2023
@trofi trofi deleted the gcc-13-mcfgthreads branch June 11, 2023 17:04
reckenrode added a commit to reckenrode/nix-packages that referenced this pull request Jul 7, 2023
@ghost ghost mentioned this pull request Jul 14, 2023
12 tasks
Comment on lines +20809 to +20815
threadsCross_pre_gcc_13 = if stdenv.targetPlatform.isMinGW && !(stdenv.targetPlatform.useLLVM or false)
then {
# other possible values: win32 or posix
model = "mcf";
# For win32 or posix set this to null
package = targetPackages.windows.mcfgthreads_pre_gcc_13 or windows.mcfgthreads_pre_gcc_13;
} else { };
Copy link

Choose a reason for hiding this comment

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

Please stop copy-pasting code like this. Please.

#243394

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to revert. Thanks.

Copy link

Choose a reason for hiding this comment

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

I'm not asking for permission to revert this, I'm asking you to stop doing it.

When you duplicate code like this, eventually either:

  1. Somebody will change one copy and forget to change the other.
  2. Somebody will deliberately change only one, but leave no explanation of why they didn't change the other.

Either way, if the duplication hasn't been noticed before one of these happens, we don't know which of them happened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consider a bit if I should stop contributing at all. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed myself from the org.

Copy link
Member

Choose a reason for hiding this comment

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

@trofi please reconsider your decision, I don't think @amjoseph-nixpkgs has any ill intentions.

I know that it can be frustrating when higher standards for code are asked.

We all struggle between perfect and shipping, but let's leave the room for discussions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 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.

2 participants