Skip to content

Conversation

@kastiglione
Copy link
Contributor

@kastiglione kastiglione commented Jan 5, 2018

Update chisel for the 1.7.1 release.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@kastiglione
Copy link
Contributor Author

@keith

@kastiglione kastiglione changed the title Update chisel to version 1.7.0 Update chisel to version 1.7.1 Jan 5, 2018
@kastiglione
Copy link
Contributor Author

Sorry for the thrash on this. I don't have an El Capitan machine to test with.

@ilovezfs ilovezfs changed the title Update chisel to version 1.7.1 chisel 1.7.1 Jan 6, 2018
@ilovezfs
Copy link
Contributor

ilovezfs commented Jan 6, 2018

code signed? How is anything being code signed here?

@kastiglione
Copy link
Contributor Author

@ilovezfs By default, Xcode code signs simulator binaries. Chisel now includes a library that aids debugging, and that library is loaded dynamically into the app being debugged. This library is built by Xcode and so it too is code signed. It's not evident in this recipe, but the make command is calling xcodebuild, and it's xcodebuild that does the signing.

@kastiglione
Copy link
Contributor Author

I will update the comment in the recipe.

@ilovezfs
Copy link
Contributor

ilovezfs commented Jan 6, 2018

There are no signing certs installed on Jenkins so I'm still not clear what we're talking about.

@kastiglione
Copy link
Contributor Author

Simulator builds use ad-hoc code signing, no signing identity is required.

@ilovezfs
Copy link
Contributor

ilovezfs commented Jan 6, 2018

If this is running make it's no longer bottle :unneeded

@kastiglione
Copy link
Contributor Author

Sounds good. How can I verify locally that a bottled install of this Chisel update works? I'd like to do that before this gets committed.

# == LD_DYLIB_INSTALL_NAME Explanation ==
# This make invocation calls xcodebuild, which in turn performs ad hoc code
# signing. Note that ad hoc code signing does not need signing identities.
# Brew will update binaries to ensure their internal paths are usable, but
Copy link
Contributor

Choose a reason for hiding this comment

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

The brew test bot isn't liking this one particular space at the very end! Can you remove it real quick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect! We'll let the brew test bot do it's thing now. 😉

Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

CI checks are a go, so it's all good with me!

@kastiglione
Copy link
Contributor Author

thanks @vinyldarkscratch. Do you know if there's a way I can install the bottle built by CI? I want to do a dry run. I don't see anything in brew install to specify a bottle location.

@queengooborg
Copy link
Contributor

By default, Homebrew will install the bottle if it can find one in the formula (you can specify otherwise with the --build-from-source option). Since this PR hasn't been merged yet, the test bot hasn't added the bottles, and as such there are none just yet!

@kastiglione
Copy link
Contributor Author

ok thanks. I downloaded the bottle from CI, and codesign --verify Chisel results in:

Chisel: invalid signature (code or signature have been modified)

I don't know what the cause is.

Is it an option to have this recipe be built by users and avoid bottling? The library is small, the build is quick.

Alternatively, the best solution may be to manually call codesign in post_install, to ensure it's always got a valid signature.

@queengooborg
Copy link
Contributor

@kastiglione There is a way, yes, as described in the Formula class documentation. You can add bottle :disable, "reasons" to your formula (which I noticed you had at one point, which leads me to the following part). However, formulae where the upstream URL breaks or moves frequently, require compile or have a reasonable amount of patches/resources should be bottled. Since chisel requires compilation, it should be bottled.

How can I verify locally that a bottled install of this Chisel update works?

If you pour chisel with the edits on your local copy, does everything function the way you'd expect? If so, then the bottled version should work perfectly. Essentially, the bottle is grabbing the pre-compiled version of the library and all of it's files, and pouring them into a bottle to ship off to the computers. (Couldn't help but keep up the beer analogy a bit. 😛)

For the codesigning, honestly I can't really say anything in regards to it. I can only quote @ilovezfs in that "there are no signing certs installed on Jenkins," so honestly it seems like codesigning doesn't even happen aside from what the compiler does?

Bottom line, if an install of Chisel on your local machine seems to work as you expect, then I think this PR is ready to be pulled. (That's when we can test to make sure the bottle functions, admittedly.) Of course, that's just my opinion!

@kastiglione
Copy link
Contributor Author

kastiglione commented Jan 10, 2018

If you pour chisel with the edits on your local copy, does everything function the way you'd expect?

When I run brew install chisel with these changes, it does work. (aside: I don't know what pour chisel is)

That's when we can test to make sure the bottle functions, admittedly.

My initial test (using the bottle built by CI) indicates the bottle won't work, but maybe my test was flawed. Even if the bottled library is invalid, it will not be a regression. The library is new, all existing features are independent. Also, I am ready with next actions if the bottle does prove to be invalid, so let's go for it.

@queengooborg
Copy link
Contributor

pour chisel

Eek! I'm so used to my aliases for different commands (pour = brew install), I keep forgetting that it's not what everyone uses! My bad... 😅

And, yeah, I think that the best next step is to have this PR pulled, and then make any changes from there as needed. 😉

@kastiglione
Copy link
Contributor Author

@ilovezfs Does this look acceptable?

@ilovezfs
Copy link
Contributor

@BrewTestBot test this please

@ilovezfs ilovezfs added the ready to merge PR can be merged once CI is green label Jan 16, 2018
@ilovezfs
Copy link
Contributor

Thanks for the upgrade @kastiglione! Shipped.

@ilovezfs ilovezfs closed this in 7f9c6db Jan 16, 2018
@kastiglione kastiglione deleted the patch-1 branch January 16, 2018 19:44
@kastiglione
Copy link
Contributor Author

Thanks for the reviews and guidance @ilovezfs

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

ready to merge PR can be merged once CI is green

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants