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.

Sign up for GitHub

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

Native post install script #1251

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Native post install script #1251

wants to merge 30 commits into from
+253 −22

Conversation

tatchi
Copy link
Contributor

@tatchi tatchi commented Dec 29, 2020

Address #1209

tatchi
tatchi commented Dec 29, 2020
View reviewed changes
esy-native-install-npm-release/EsyNativeInstallNpmRelease.re Outdated
Comment on lines 160 to 168
| Ok(_) => print_endline("Done!")
| Error(`NoBuildFound) => Printf.eprintf("No build found!")
| Error(`ReleaseAlreadyInstalled) =>
Printf.eprintf("Release already installed!")
| Error(`Msg(msg)) => Printf.eprintf("%s", msg)
| Error(`EsyLibError(err)) =>
Printf.eprintf("%s", EsyLib.Run.formatError(err))
| Error(`CommandError(cmd, status)) =>
Bos.Cmd.pp(Format.err_formatter, cmd)
};
Copy link
Contributor Author

@tatchi tatchi Dec 29, 2020 •
edited

Choose a reason for hiding this comment

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

I wanted to have the main function to return a result type with the error part being all variants so we could pattern match on them and decide what to do. Since we actually do nothing more than printing the error in every case, I might have made this more complicated than needed.

Happy to refactor and use the RunAsync.error(string) function to report these errors if you prefer

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be great! As much code/pattern reuse as possible.

@tatchi
Copy link
Contributor Author

tatchi commented Dec 29, 2020 •
edited

Although I included @slowtest in my latest commit, I was not able to trigger them. Looks like azure is using a different commit message: https://developercommunity.visualstudio.com/content/problem/1002300/buildsourceversionmessage-for-prs-is-set-to-info-f.html

tatchi
tatchi commented Dec 29, 2020
View reviewed changes
esy-native-install-npm-release/EsyNativeInstallNpmRelease.re
Comment on lines +125 to +127
switch (System.Platform.host, System.Arch.host) {
| (Darwin, Arm64) =>
print_endline("Detected macOS arm64. Signing binaries...");
Copy link
Contributor Author

@tatchi tatchi Dec 29, 2020 •
edited

Choose a reason for hiding this comment

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

I saw after that you're doing it differently here:

esy/esy-build-package/Build.re

Line 372 in 560642c

let isBigSurArm =

I have the feeling that my solution is better but I might be wrong ofc. Let me know what you think and I can adapt :)

Copy link
Member

Choose a reason for hiding this comment

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

I completely missed EsyLib.System - thanks!

EsyLib.System internally runs uname too. Teeny tiny nit - it runs uname twice. Definitely, doesnt justify reinventing it again.

+1 to your approach.

@ManasJayanth
Copy link
Member

ManasJayanth commented Jan 7, 2021 •
edited

@tatchi The CI failure is because of esyNativeInstallNpmRelease not being install via yarn/npm in the Alpine build. Can you see what's going on there? Thankfully, the issue can be repro'ed locally

@tatchi tatchi force-pushed the native-post-install branch 2 times, most recently from d3272d7 to b295f73 Compare January 13, 2021 20:55
@tatchi tatchi force-pushed the native-post-install branch 9 times, most recently from da3356e to 1797b2e Compare February 10, 2021 13:29
@Et7f3
Copy link
Contributor

Et7f3 commented Oct 7, 2022

What are the blocker of this PR ?

@tatchi
Copy link
Contributor Author

tatchi commented Oct 16, 2022

What are the blocker of this PR ?

It's been a while so I'm not totally sure but from what I remember, something was not working on CI. I see that @ManasJayanth force-pushed the last, so maybe he has better clue?

@ManasJayanth ManasJayanth mentioned this pull request Jun 6, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ManasJayanth ManasJayanth ManasJayanth left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tatchi @ManasJayanth @Et7f3