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.

Sign up for GitHub

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

Jump to bottom

bash-completion: fix build on darwin (#107768) #107813

Merged
merged 1 commit into from Dec 29, 2020
Merged

bash-completion: fix build on darwin (#107768) #107813

merged 1 commit into from Dec 29, 2020

Conversation

rb2k
Copy link
Contributor

@rb2k rb2k commented Dec 28, 2020 •
edited

Motivation for this change

As discussed in #107768, macOS APFS and HFS+ seem to have issues extracting some of the test fixtures because of filesystem issues (case sensitivity, binary representation of characters, ...). This means that people currently won't be able to build things from source on macOS.
I'm a bit confused as to how the binary caches with this version got created.

By switching to fetchurl, we will still validate the tarball checksum itself while being able to compile on mac and linux.
It's not the cleanest solution, but it does work.

I tried to look into things like cleanSource / cleanSourceWith, but I'm not 100% sure that would solve the issue at hand.

Comparison:

Grab the new files:

% nix-build default.nix -A bash-completion
/nix/store/br1vvv70z4icia58fzlcw1nxid28ww62-bash-completion-2.11
% pushd /nix/store/br1vvv70z4icia58fzlcw1nxid28ww62-bash-completion-2.11; find . > ~/new.txt; popd

Grab the current binary results:

% nix-env -i bash-completion
warning: there are multiple derivations named 'bash-completion-2.11'; using the first one
installing 'bash-completion-2.11'
these paths will be fetched (0.14 MiB download, 0.97 MiB unpacked):
  /nix/store/jyniimfqps4b1dgm2ya0hqyilc7g4y8v-bash-completion-2.11
copying path '/nix/store/jyniimfqps4b1dgm2ya0hqyilc7g4y8v-bash-completion-2.11' from 'https://cache.nixos.org'...
building '/nix/store/xbfs4rdn10ir4bjbnqahkzbrc0s6fs5a-user-environment.drv'...
created 257 symlinks in user environment
% pushd /nix/store/jyniimfqps4b1dgm2ya0hqyilc7g4y8v-bash-completion-2.11; find . > ~/binary.txt; popd
/nix/store/jyniimfqps4b1dgm2ya0hqyilc7g4y8v-bash-completion-2.11 ~/workspace/nixpkgs
~/workspace/nixpkgs

Compare the filenames:

% diff -qs ~/binary.txt ~/new.txt
Files /Users/mseeger/binary.txt and /Users/mseeger/new.txt are identical

Sanity check:

% head -n 5 ~/binary.txt
.
./etc
./etc/profile.d
./etc/profile.d/bash_completion.sh
./share
% head -n 5 ~/new.txt   
.
./etc
./etc/profile.d
./etc/profile.d/bash_completion.sh
./share
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@rb2k rb2k force-pushed the bashcompletion branch 2 times, most recently from 19da515 to 6af2296 Compare December 28, 2020 16:15
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Dec 28, 2020
@rb2k rb2k mentioned this pull request Dec 28, 2020
Closed
@ofborg ofborg bot requested review from KamilaBorowska and peti December 28, 2020 16:31
@rb2k
Copy link
Contributor Author

rb2k commented Dec 28, 2020 •
edited

@SuperSandro2000 noticed that this derivation caused issues on the last merge: #101028

I'm trying to run nixpkgs-review, but that might not finish for a while...

macOS:

[0/1699 built, 1854/3460 copied (10789.7/25415.3 MiB), 2430.0/7720.6 MiB DL]

Linux:

[0/14753 built, 1/156/14619 copied (1424.5/161995.0 MiB), 1253.8/69348.2 MiB DL]

I'll run one that just builds bash-completion.

@rb2k
Copy link
Contributor Author

rb2k commented Dec 28, 2020

Result of nixpkgs-review pr 107813 run on x86_64-darwin 1

1 package built:
  • bash-completion

@rb2k
Copy link
Contributor Author

rb2k commented Dec 28, 2020

Result of nixpkgs-review pr 107813 run on x86_64-linux 1

1 package built:
  • bash-completion

@KamilaBorowska
Copy link
Member

KamilaBorowska commented Dec 28, 2020 •
edited

Please change this pull request to target staging branch (not master) as this is mass rebuild.

@rb2k rb2k changed the base branch from master to staging December 28, 2020 17:31
@rb2k
Copy link
Contributor Author

rb2k commented Dec 28, 2020 •
edited

Almost there, just need a second to properly rebase that on staging. The mercurial all day at $DAYJOB is showing :)

EDIT: Done!

KamilaBorowska
KamilaBorowska approved these changes Dec 28, 2020
View reviewed changes
Copy link
Member

@KamilaBorowska KamilaBorowska left a comment •
edited

Choose a reason for hiding this comment

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

I don't have macOS myself to test this on macOS, but this looks fine. It's not ideal, but it's an acceptable workaround I feel.

@rb2k rb2k requested a review from prusnak December 28, 2020 23:04
@jonringer
Copy link
Contributor

the files which caused issues were part of the testing framework.. do we still have tests?

@jonringer
Copy link
Contributor

why does this cause so many rebuilds...

jonringer
jonringer approved these changes Dec 29, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer 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

package outputs are the same

[09:33:02] jon@nixos ~/projects/nixpkgs (master)
$ diff new.txt old.txt
[09:33:06] jon@nixos ~/projects/nixpkgs (master)

@rb2k
Copy link
Contributor Author

rb2k commented Dec 29, 2020 •
edited

the files which caused issues were part of the testing framework.. do we still have tests?

They are there, probably mangled on OSX. I don't think the current derivation executes any of the tests.
The derivation mentions:

  # tests are super flaky unfortunately, and regularily break.
  # let's disable them for now.
  doCheck = false;

why does this cause so many rebuilds...

Because all these packages depend on bash-completions, presumably because they ship their own. That's why it's targeted at staging rather than master

@jonringer jonringer merged commit 1d83fc2 into NixOS:staging Dec 29, 2020
@jonringer
Copy link
Contributor

we actually never had tests ;(, so this is just an improvement for mac users

@jonringer
Copy link
Contributor

jonringer commented Dec 29, 2020 •
edited

Because all these packages depend on bash-completions, presumably because they ship their own. That's why it's targeted at staging rather than master

no, it's because dconf "links" against it for some reason, thus gets used as a build dependencies in a lot of glib / gnome related projects.

EDIT: Nevermind, they do a check to see how they need to export the bash-completions: https://gitlab.gnome.org/GNOME/dconf/-/blob/master/meson.build#L51

@rb2k rb2k deleted the bashcompletion branch December 29, 2020 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prusnak prusnak prusnak approved these changes

@KamilaBorowska KamilaBorowska KamilaBorowska approved these changes

@jonringer jonringer jonringer approved these changes

@peti peti Awaiting requested review from peti

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@aanderse aanderse Awaiting requested review from aanderse

@romildo romildo Awaiting requested review from romildo

@kalbasit kalbasit Awaiting requested review from kalbasit

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 501-1000 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rb2k @KamilaBorowska @jonringer @prusnak @SuperSandro2000