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

Allow for disabling codesigning at build time #6952

Closed
lilyball opened this issue Apr 27, 2020 · 13 comments
Closed

Allow for disabling codesigning at build time #6952

lilyball opened this issue Apr 27, 2020 · 13 comments
Milestone

Comments

@lilyball
Copy link
Contributor

Fish 3.1.1 now codesigns executables during build (see d0a67e3). Unfortunately this breaks compilation in Nix as we don't have access to /usr/bin/codesign. We can patch the source for now but it would be nice if we had an official way to disable this. Perhaps by checking if MAC_CODESIGN_ID was set to the empty string.

@ridiculousfish
Copy link
Member

With 3a47db7, if the CMake variable MAC_CODESIGN_ID is falsey, then code signing is disabled.

From CMake one might pass -DMAC_CODESIGN_ID=OFF (or 0, etc).

Thanks for filing!

@Darkshadow2
Copy link
Contributor

@ridiculousfish That change will turn off code signing, but can you also restore the ability to use an already compiled PCRE (system PCRE) if it's turned off as well? It just needs IF (NOT APPLE OR NOT MAC_CODESIGN_ID) in cmake/PCRE2.cmake.

@lilyball
Copy link
Contributor Author

Better still would be to introduce a different cmake variable. I didn't realize fish always used bundled pcre on Apple platforms.

@lilyball
Copy link
Contributor Author

Oh wait the reason for this is literally codesigning. Ok then, using a system PCRE when codesigning is disabled seems like a reasonable toggle.

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Apr 28, 2020
The CMake variable FISH_USE_SYSTEM_PCRE2 now controls whether fish uses
system PCRE2 or the bundled version. The default is to use the system
version, unless no such version is found, or unless it is a macOS build
with code signing. Note the default behavior has not changed.

Fixes fish-shell#6952
@novadev94
Copy link

I had another problem while trying to build Fish from source (without disabling codesign) when gettext is installed via brew

There is a programmable error in your configuration file:

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/sphinx/config.py", line 348, in eval_config_file
    execfile_(filename, namespace)
  File "/usr/local/lib/python3.7/site-packages/sphinx/util/pycompat.py", line 81, in execfile_
    exec(code, _globals)
  File "/oss/fish-shell/doc_src/conf.py", line 62, in <module>
    ("fish_indent", "--version"), stderr=subprocess.STDOUT
  File "/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/lib/python3.7/subprocess.py", line 411, in check_output
    **kwargs).stdout
  File "/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/lib/python3.7/subprocess.py", line 512, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '('fish_indent', '--version')' died with <Signals.SIGABRT: 6>.

Tried running built fish_indent to see what happens

dyld: Library not loaded: /usr/local/opt/gettext/lib/libintl.8.dylib
  Referenced from: /oss/fish-shell/./fish_indent
  Reason: no suitable image found.  Did find:
	/usr/local/opt/gettext/lib/libintl.8.dylib: code signature in (/usr/local/opt/gettext/lib/libintl.8.dylib) not valid for use in process using Library Validation: mapped file has no cdhash, completely unsigned? Code has to be at least ad-hoc signed.
	/usr/local/opt/gettext/lib/libintl.8.dylib: stat() failed with errno=1
	/usr/local/Cellar/gettext/0.20.2_1/lib/libintl.8.dylib: code signature in (/usr/local/Cellar/gettext/0.20.2_1/lib/libintl.8.dylib) not valid for use in process using Library Validation: mapped file has no cdhash, completely unsigned? Code has to be at least ad-hoc signed.
Abort trap: 6

A quick check if libintl.8.dylib is signed

> codesign -d -vvv /usr/local/Cellar/gettext/0.20.2_1/lib/libintl.8.dylib
/usr/local/Cellar/gettext/0.20.2_1/lib/libintl.8.dylib: code object is not signed at all

Of course, turning off codesign fixed it.

@ridiculousfish
Copy link
Member

I would hazard that disabling codesigning outright is the right thing for Nix, and other Mac package managers as well.

@lilyball
Copy link
Contributor Author

I would hazard that disabling codesigning outright is the right thing for Nix, and other Mac package managers as well.

I agree. It's definitely required for Nix, and probably a good idea for Homebrew. We just need to make sure we handle cases like allowing system pcre when codesign is off.

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Apr 29, 2020
The CMake variable FISH_USE_SYSTEM_PCRE2 now controls whether fish uses
system PCRE2 or the bundled version. The default is to use the system
version, unless no such version is found, or unless it is a macOS build
with code signing. Note the default behavior has not changed.

Fixes fish-shell#6952
ridiculousfish added a commit that referenced this issue Apr 29, 2020
The CMake variable FISH_USE_SYSTEM_PCRE2 now controls whether fish uses
system PCRE2 or the bundled version. The default is to use the system
version, unless no such version is found, or unless it is a macOS build
with code signing. Note the default behavior has not changed.

Fixes #6952
@ridiculousfish
Copy link
Member

ridiculousfish commented Apr 29, 2020

A separate CMake variable makes sense to me; I added FISH_USE_SYSTEM_PCRE2 in b88b6ea. By default this is off if building on Mac with codesigning enabled, or if no system PCRE2 was found, and otherwise on. The hope is it should not be necessary for Nix to set it explicitly.

@zanchey
Copy link
Member

zanchey commented May 24, 2020

I did try and get this into the Homebrew formula, but I can't make it work except with the HEAD build.

@danzimm
Copy link

danzimm commented Jun 16, 2020

Hey, sorry to dig this up, but it looks like the issue raised in #6952 (comment) wasn't quite addressed by #6952 (comment) - if you have gettext installed via homebrew then it looks like fish still tries to link against the unsigned copy. I'm not sure what the right fix is here- I have no clue if macos ships with gettext usually. I've worked around the issue by specifying -DWITH_GETTEXT=NO when running cmake, so no urgency here. Just wanted to flag and give a workaround in case someone else runs into the issue

@Darkshadow2
Copy link
Contributor

@danzimm As @ridiculousfish pointed out in the comment after that, disabling codesigning is the right thing to do if you're using a library not already included in macOS (and gettext isn't part of macOS). Or at least is the right thing to do until/if package managers start codesigning libraries.

Disabling localization works too in this case, as you already found out.

@zanchey
Copy link
Member

zanchey commented Jun 16, 2020

The Homebrew formula needs a patch:

--- a/Formula/fish.rb
+++ b/Formula/fish.rb
@@ -30,6 +30,7 @@ class Fish < Formula
       -Dextra_completionsdir=#{HOMEBREW_PREFIX}/share/fish/vendor_completions.d
       -Dextra_confdir=#{HOMEBREW_PREFIX}/share/fish/vendor_conf.d
       -DSED=/usr/bin/sed
+      -DMAC_CODESIGN_ID=OFF
     ]
     system "cmake", ".", *std_cmake_args, *args
     system "make", "install"

I haven't gotten around to submitting that upstream though. I can't work out how to make it only apply to HEAD builds (not supported before that).

@danzimm
Copy link

danzimm commented Jun 17, 2020

@Darkshadow2 gotcha! I couldn’t quite grok that comment, thanks for re-explaining!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants