-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Packaging enhancements #32
Conversation
6a1ad55
to
fdef5ec
Compare
adc7f7d
to
dd6bf30
Compare
fa8a7aa
to
520f15b
Compare
520f15b
to
cb01a08
Compare
954c6b7
to
7c8167e
Compare
5fcfd60
to
ebb6479
Compare
ebb6479
to
1d4d4b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good overall, thanks @SleepyLeslie. Some questions.
@@ -29,9 +29,6 @@ jobs: | |||
with: | |||
submodules: true | |||
|
|||
- name: check bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this check removed? It was checking something pretty subtle and weird but I see there are no comments as to what exactly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not against removing, just checking why)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the code and couldn't understand why this check needs to be present. It was doing no useful work (basically if true; then echo ok; fi
then repeat for another time), and there was no comment to explain its significance. I thought it was legacy leftovers not cleaned up. I double checked my CI and packaging runs and this step never failed (I would be surprised if it did). My guess is it might be checking whether the Windows association step worked (script is run with bash rather than powershell), and was likely added for debugging purposes, so I removed it. If anything breaks in the future I can always come back to it.
Please do share any extra context you have on what the problem was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was likely something like that, figuring out the association, and that a good version of bash was running. Doing bash-like stuff in windows was delicate, and I could easily imagine it breaking in future again. Still, since these checks have no comments, fine to remove them. Thanks for explaining.
@@ -108,9 +121,6 @@ | |||
"role": "Editor", | |||
"icon": "core/static/icons/gristdoc" | |||
} | |||
], | |||
"nsis": { | |||
"perMachine": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what this was?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to this documentation, this option does nothing when oneClick
is true
(default, we didn't change it), so I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI otherwise this option tells NSIS to skip the installation mode page and always installs Grist Desktop system-wide. This might not be a good idea in restricted environments where user does not have admin privileges, but we now have the portable build so this shouldn't matter much?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this was the hard-to-find solution to some gnarly problem, but we shouldn't keep it out of superstition. Someone will shout if it turns out to have some effect that was important, and then we can document the reason better next time :-). Cheers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SleepyLeslie ! Good to land.
@@ -29,9 +29,6 @@ jobs: | |||
with: | |||
submodules: true | |||
|
|||
- name: check bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was likely something like that, figuring out the association, and that a good version of bash was running. Doing bash-like stuff in windows was delicate, and I could easily imagine it breaking in future again. Still, since these checks have no comments, fine to remove them. Thanks for explaining.
@@ -108,9 +121,6 @@ | |||
"role": "Editor", | |||
"icon": "core/static/icons/gristdoc" | |||
} | |||
], | |||
"nsis": { | |||
"perMachine": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this was the hard-to-find solution to some gnarly problem, but we shouldn't keep it out of superstition. Someone will shout if it turns out to have some effect that was important, and then we can document the reason better next time :-). Cheers.
node_modules
is now cached, saving 3min30s of build time and an additional 2min for Windows x86 builds.