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

Jump to bottom

fish: 3.1.0 -> 3.1.1 #86136

Merged
merged 2 commits into from Apr 28, 2020
Merged

fish: 3.1.0 -> 3.1.1 #86136

merged 2 commits into from Apr 28, 2020

Conversation

cole-h
Copy link
Member

@cole-h cole-h commented Apr 27, 2020 •
edited

fish-shell/fish-shell@3.1.0...3.1.1

The patch we had to use for Apple SDKs was merged upstream, so it can be
dropped. I ran nixpkgs-fmt, and removed the with stdenv.lib; scope
expander.

Additionally, did a little bit of cleanup. I plan on refactoring this
more down the line, but this'll do for now.

I finally figured out why we use fetchurl for the tagged release: the
published release tarballs contain a version file, which the
build_tools/git_version_gen.sh script reads (and uses as the version
if it exists). The other thing it contains are pre-generated docs for
various fish builtins. I've expanded the comment to document this so
nobody is as confused as I was when I first saw it. (Though I plan to
change this and add sphinx as a native build input in order to build the
docs ourselves.)

Motivation for this change
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.

cc @lilyball: Mind testing on macOS, to ensure we don't run into a similar issue to last time?
cc @Profpatsch: Is the withTests stuff still necessary? Can I drop it, or is there something you'd like done with it?

EDIT: I also verified that the NixOS module's patch still applies, but if somebody could determine for certain whether or not anything needs to be fixed there, as well, I'd appreciate it.

@cole-h
fish: 3.1.0 -> 3.1.1
a43ae05 
fish-shell/fish-shell@3.1.0...3.1.1

The patch we had to use for Apple SDKs was merged upstream, so it can be
dropped. I ran nixpkgs-fmt, and removed the `with stdenv.lib;` scope
expander.

Additionally, did a little bit of cleanup. I plan on refactoring this
more down the line, but this'll do for now.

I finally figured out why we use `fetchurl` for the tagged release: the
published release tarballs contain a version file, which the
`build_tools/git_version_gen.sh` script reads (and uses as the version
if it exists). The other thing it contains are pre-generated docs for
various `fish` builtins. I've expanded the comment to document this so
nobody is as confused as I was when I first saw it. (Though I plan to
change this and add sphinx as a native build input in order to build the
docs ourselves.)
@ofborg ofborg bot requested a review from ocharles April 27, 2020 17:02
@worldofpeace
Copy link
Contributor

@GrahamcOfBorg build fish

@lilyball
Copy link
Member

Build fails on macOS due to fish-shell/fish-shell@d0a67e3:

  Scanning dependencies of target fish_indent
  [ 95%] Building CXX object CMakeFiles/fish_indent.dir/src/fish_indent.cpp.o
  [ 96%] Building CXX object CMakeFiles/fish_indent.dir/src/print_help.cpp.o
  [ 96%] Linking CXX executable fish_indent
  /nix/store/i9jh1yxfinl0pa5y2c9h2z5i0sdhaf7q-bash-4.4-p23/bin/bash: codesign: command not found
  make[2]: *** [CMakeFiles/fish_indent.dir/build.make:121: fish_indent] Error 127
  make[2]: *** Deleting file 'fish_indent'
  make[1]: *** [CMakeFiles/Makefile2:662: CMakeFiles/fish_indent.dir/all] Error 2
  make: *** [Makefile:147: all] Error 2

Since we presumably don't want to break the sandbox just for codesigning (can you even Ad-Hoc codesign without a developer account?) we're going to have to disable it. I'll file an upstream issue about having an official way to turn it off, but until then I guess we can just patch CMakeLists.txt.

@lilyball
Copy link
Member

Upstream issue filed as fish-shell/fish-shell#6952.

@cole-h
fish: disable codesigning
87b02f7 
We don't have access to the codesign binary.
@adisbladis
Copy link
Member

Looks like the code signing was patched out :)
@GrahamcOfBorg build fish

@Profpatsch
Copy link
Member

Do you want to add yourself as a maintainer?

@Profpatsch
Copy link
Member

cc @Profpatsch: Is the withTests stuff still necessary? Can I drop it, or is there something you'd like done with it?

The withTests can be replaced by putting the test into passthru.tests and making it a runCommand instead of a string. withTests is a primitive that I never got around to upstreaming.

@adisbladis adisbladis merged commit f0fbce6 into NixOS:master Apr 28, 2020
@cole-h cole-h deleted the fish branch April 28, 2020 15:05
@lilyball
Copy link
Member

There's now a way on fish master to disable it: fish-shell/fish-shell@3a47db7

So we can set that new value on the next release.

@Profpatsch
Copy link
Member

Profpatsch commented May 4, 2020 via email

@cole-h
Copy link
Member Author

cole-h commented May 4, 2020 •
edited

The only reason the test wasn't added was because it fails. I've been working off and on to try and fix it up, but the tempfile appears to never get created: cat: /build/web_configyaz_apca.html: No such file or directory.

EDIT: That was easy. Just had to look at the docs for tempfile -- just needed to add delete=False to the NamedTempFile function.

@cole-h
Copy link
Member Author

cole-h commented May 4, 2020

Test fixed in followup here: #86802.

@Profpatsch
Copy link
Member

Profpatsch commented May 4, 2020 via email

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

@ocharles ocharles Awaiting requested review from ocharles

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cole-h @worldofpeace @lilyball @adisbladis @Profpatsch