Properly quote unix launch scripts #199
Merged
Conversation
This file contains 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
linux scripts
Properly quote osx fetch_chromium Properly quote osx build_blink
ayjayt
requested changes
Jul 26, 2024
There was a problem hiding this comment.
Hi @BlueDrink9, you're 100% right. We have 3 PR's up for a complete overhaul of the build system. I do believe I properly quoted all of my bash variables. Win + Mac are in internal debugging and should be out early next week.
- All platform's build scripts are consolidated into a series of bash scripts
00-NAME.shthrough13-NAME.sh. There are more complicated helper scripts xx-NAME.sh if you're interested in a developer environment, but I'm not sure if those are cross platform. 00-.sh through 13-.sh are. - It is accompanied with a update of the C++ API to support concurrency (although kaleido.py doesn't support asynchronous exports yet) and the newer versions of chromium (>126). I didn't fix the build for the old version of chromium master currently supports (88) because it has a bunch of CVE's anyway and it's 3rd party dependencies have degraded.
I will do my best over the coming weeks to litter READMEs through the directories and better comment the code.
Thank you for the PR. I include one question but if we're really just properly quoting "" we can accept it before accepting the larger overhaul without issue.
|
Don't worry!
El vie, 26 de jul de 2024, 4:14 p. m., Silico_Biomancer <
***@***.***> escribió:
… ***@***.**** commented on this pull request.
------------------------------
In repos/mac_scripts/build_kaleido
<#199 (comment)>:
> if [ ! -f "out/Kaleido_mac_$KALEIDO_ARCH/kaleido" ]
then
echo "Error: Kaleido executable was not built";
- exit 1
Indeed it is, terribly sorry. Quotes should be the only changes!
—
Reply to this email directly, view it on GitHub
<#199 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHHLRFIMXI6EDANB6WJTXW3ZOKU35AVCNFSM6AAAAABLNP4XTGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBSGY4DCOJYGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Fixed that now. I also see my editor automatically did some formatting and created errant newlines, apologies for that extra messiness in the diff. |
|
theres really no issue, i appreciate it
…On Tue, Jul 30, 2024 at 6:33 AM Silico_Biomancer ***@***.***> wrote:
Fixed that now. I also see my editor automatically did some formatting and
created errant newlines, apologies for that extra messiness in the diff.
—
Reply to this email directly, view it on GitHub
<#199 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHHLRFJNEPWV4H5BKHRH2LDZO5TWTAVCNFSM6AAAAABLNP4XTGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJYGAZDKOBTHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
gvwilson
added
community
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a classic bash bug. Without quotes, any spaces in the directory that kaleido is installed into will cause the commands to split into multiple arguments. This change fixes #78, a long-standing (3 years) and annoying issue for people using per-project venvs with spaces in the project name.