fix code-signing on macOS#15592
Merged
adamjstewart merged 18 commits intospack:developfrom Apr 22, 2020
Merged
Conversation
Contributor
|
Comments:
variant('lldb', default='all',
description='Enable LLDB with debug server',
values=('none', 'no_debug_server', 'all'))
if sys.platform != "darwin":
conflicts('lldb=no_debug_server')
|
Contributor
Author
|
If we switch I've tried to clean up the previous mistakes but I don't have a mac to test this on. If this still looks too convoluted I can implement the multi-variant approach. |
Contributor
|
If you can’t test it, why are you submitting it? |
Contributor
Author
|
I was planning on testing this on a mac at the library but since that can't be done for the next few months, this PR won't be updated. Sincere apologies for wasting reviewer's time. I'll be more stringent with testing PR's I submit in the future. |
Contributor
|
I'm sorry for my late reply. I don't have access to my Mac machine where I could test this. Do you recommend using a virtual machine to test it? |
Contributor
|
Hi all, $ spack install llvm
==> 27802: Installing llvm
==> Warning: microarchitecture specific optimizations are not supported yet on mixed compiler toolchains [check clang@11.0.3-apple for further details]
==> Fetching https://github.com/llvm/llvm-project/archive/llvmorg-10.0.0.tar.gz
#################################################################################################################################################################################################################### 100.0% # -=O=- # # #
==> Staging archive: /var/folders/5p/x7ftfty12yndrs3p6ntlk1jc0000gp/T/LDianaAmorim/spack-stage/spack-stage-llvm-10.0.0-t7zrqbqbhjanwncpega7vqzwjkk327wk/llvmorg-10.0.0.tar.gz
==> Created stage in /var/folders/5p/x7ftfty12yndrs3p6ntlk1jc0000gp/T/LDianaAmorim/spack-stage/spack-stage-llvm-10.0.0-t7zrqbqbhjanwncpega7vqzwjkk327wk
==> No patches needed for llvm
==> 27802: llvm: Building llvm [CMakePackage]
==> 27802: llvm: Executing phase: 'cmake'
Password:I tried switching to @s-sajid-ali remote and the $ spack install llvm
==> Error: name 'self' is not definedThis is probably an obvious error that I am doing. But what am I missing? In this branch I have: $git log
commit 24a72cdd5c967fe229cb8ffc2bae26f0fdd64377 (HEAD -> llvm_code_signing, ssajid/llvm_code_signing)
Author: Sajid Ali <sajidsyed2021@u.northwestern.edu>
Date: Fri Mar 20 15:02:30 2020 -0500
clarify
...This was with: $ spack debug report
* **Spack:** 0.14.1-603-91e22b8ae
* **Python:** 3.7.6
* **Platform:** darwin-catalina-skylakeThank you for trying to fix this! |
LDAmorim
reviewed
Apr 13, 2020
24a72cd to
06eb265
Compare
Contributor
|
Hi, Thank you @s-sajid-ali and @adamjstewart . |
Contributor
Author
|
As per [1], [2] the easiest way the install the missing library is via Does this error occur only when you build with Possibly related #7151 ? |
Contributor
|
Hi @s-sajid-ali ! Thanks for the reply. I will try to link spack llvm to the Xcode built library then. I do have the libraries installed, in I got it using your branch which did not request the password. Thanks |
Contributor
Author
|
In that case, the absence of the library isn't the issue then. Just to confirm you have both xcode and command line tools installed ? External packages as described in |
Contributor
|
Yes I do have both installed. |
Contributor
|
Thanks @s-sajid-ali ! I just tried to re-install everything with spack and clang (using gfortran to get mpich, but not using gcc/g++). I am trying to learn how to fix it with similar issues: EOSIO/eos#2392 I thought these were defined in |
Closed
Member
|
I agree, this is the right approach to this problem, no sledgehammers. |
Contributor
|
Hi all, I still can't fix the last issue I mentioned here. I tried to set: But it still led to the same error. Do you have any suggestion on how I could go around this issue? I am not able to understand how to fix this from the related issues in EOS and other packages. Thanks |
Member
|
Potentially, #16193 fixes the latter issue, so let's treat that independent of the signing patch herein. Dependencies: llvm -> Python [build] -> gettext and llvm -> binutils -> gettext |
adamjstewart
approved these changes
Apr 21, 2020
Member
adamjstewart
left a comment
There was a problem hiding this comment.
@trws and @naromero77 are our LLVM package maintainers, would like to see a review from one or both of them.
Contributor
|
I'm not sure I'm entirely comfortable with the package running the setup-codesign script. That's pretty far outside of what I'd normally expect a spack package to do, and while it might be a bad idea to run spack as root this might be a strange surprise if someone did. Is it worth it rather than just giving the user instructions if |
ax3l
reviewed
Apr 21, 2020
Contributor
|
My comments are similar to @trws, but I want to make sure I understand:
@adamjstewart Are there other Spack packages that do something similar? If so, I would be OK with the changes. If its unprecedented elsewhere in Spack, I would prefer that some error is thrown with a msg to the user to complete this step code signing step manually. @s-sajid-ali The other question I have is does this really need to be another variant. It seems like you would want to always perform this check under the following conditions: It seems like an unusual case for a variant. Usually variant are for enable a feature. Unless, I am really misunderstanding what code signing aims to accomplish. |
Member
|
Just a note: the The only thing that will happen by default on macOS is that lldb will actually work and will use the system debug server. The alternative would be to disable |
Contributor
Author
|
@trws , @naromero77 : Just to clarify, the setup script is run by default on the I made a mistake with the |
citibeth
reviewed
Apr 21, 2020
Contributor
|
@adamjstewart I discussed with @trws and I am OK with changes. BTW, should I be appearing as a review on this PR? |
Member
|
@naromero77 I think @adamjstewart has to add you to a spack organization team on github so we can select you in the github reviewer thingy. Are you a member of the github spack organization already? If you have a pending invite, it usually shows up here: https://github.com/orgs/spack/invitation |
Contributor
|
@naromero77 No, it does not look like have I have an invitation to the spack organization team. |
lorddavidiii
pushed a commit
to lorddavidiii/spack
that referenced
this pull request
Jul 2, 2020
* rebase * move if statement location * remove whitespace * spec to self.spec * switch statements as per review * fix erronous indent * add missing cmake arg * minor placement fix for cmake args * edit comment * fix erronous return * clarify conflicts with messages * remove duplicate comment * simplify logic * macos wasn't a variant, fix that * remove extra blank line * address reviewer comments on spaces
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #15410
@sethrj , @adamjstewart @LDAmorim