Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

feat: add option to skip signature on macos #1878

Merged
merged 5 commits into from
Mar 17, 2023
Merged

feat: add option to skip signature on macos #1878

merged 5 commits into from
Mar 17, 2023

Conversation

Mikescops
Copy link
Contributor

There are scenarios where one would want to sign manually or later the binaries.

I'm adding a simple option that will skip the final codesign on macos.

@Mikescops
Copy link
Contributor Author

@baparham whenever you have time to take a look here 🙌

@baparham
Copy link
Contributor

Can you come up with a test for this? There seems to be one other signature test but I'm not entirely sure how you could replicate it to test this feature and only run on mac, but I welcome your thoughts, otherwise this is untested code (even though it is simple enough to 'test' in your brain)

https://github.com/vercel/pkg/blob/main/test/test-50-signature/main.js

@Mikescops
Copy link
Contributor Author

@baparham I come up with a solution that works well on my mac, but it seems like there are no CI for macos-latest currently

@baparham
Copy link
Contributor

Ah, of course. We should fix that, are you comfortable adding to the workflow matrix in this pr?

@Mikescops
Copy link
Contributor Author

We can have a look at how bad it is, let me push it

@baparham
Copy link
Contributor

let's see! Thanks

@Mikescops
Copy link
Contributor Author

Macos is damn too smart...
image

@Mikescops
Copy link
Contributor Author

@baparham ok I made some extra fixes and I could run all tests on my computer

@Mikescops
Copy link
Contributor Author

Sounds good ✅

@Mikescops
Copy link
Contributor Author

I'd recommend dropping node12 support to lighten a bit the CI

@baparham
Copy link
Contributor

I'd recommend dropping node12 support to lighten a bit the CI

I 100% agree with this. I see no reason to support an EOL node js publicly with pkg.

I'm not sure what SLA we claim in pkg though, so this probably needs some more discussion from @robertsLando and maybe @leerob

@Mikescops
Copy link
Contributor Author

I have opened a separate issue as we can move forward on this specific PR first #1886

Is it ok for you to merge?

Copy link
Contributor

@baparham baparham left a comment

Choose a reason for hiding this comment

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

lgtm

@baparham baparham changed the title Add option to skip signature on macos feat: add option to skip signature on macos Mar 17, 2023
@Mikescops
Copy link
Contributor Author

why this merge commit?

@baparham
Copy link
Contributor

why this merge commit?

It seems the repo is set up to require the branch be up to date before I can merge it. I don't really know why, but now it says it can be merged in, which I'll do in a squash commit. Perhaps the requirement is that the checks pass on the final merged result or something against main 🤷

@baparham baparham merged commit edfdadb into vercel:main Mar 17, 2023
@Mikescops
Copy link
Contributor Author

Weird, I made a rebase on main before pushing 🤔

Thanks for the help here!

@Mikescops Mikescops deleted the feature/skip-signature-macos branch March 20, 2023 09:00
@Mikescops
Copy link
Contributor Author

@baparham @robertsLando can we tag a release? i'd need to use the new option soon 👍

@robertsLando
Copy link
Contributor

robertsLando commented Mar 28, 2023

@Mikescops Unfortunately this doesn't depends by us as we both don't have the permissions to make new releases. @baparham already sent a private message to @leerob for this, I think it will be ready for the end of this week.

BTW in the meanwhile you could simply npm install vercel/pkg#main

@Mikescops
Copy link
Contributor Author

Ok thanks, I'll wait then. I can't really do this in a production software unfortunately 😂

@robertsLando
Copy link
Contributor

@Mikescops Giving that you just need to use it to package the application (it's a dev dependency) that will give the same result as after the release so not really a big difference

@Mikescops
Copy link
Contributor Author

Not really, because it will be used in a brew Formula to package the app, and it is versioned, and I can't guarantee what will be added to the main branch here 'till i can update the brew Formula.

pcnate pushed a commit to pcnate/pkg that referenced this pull request Aug 15, 2023
* Add option to skip signature on macos

* Add unit tests for no-signature

* Add macos-latest to matrix

* Macos CI various fixes

---------

Co-authored-by: Brad Parham <baparham@gmail.com>
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants