-
Notifications
You must be signed in to change notification settings - Fork 4k
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
chore(build): fast fail PR builds if rosetta extraction fails #14946
Changes from all commits
ff7a571
0c6bbe5
31423b2
af3b4b9
b1def37
7186f98
9ea6b50
6979f5c
28477c2
62329bb
f0ed7d1
806987f
32e0242
4193fe7
12aa309
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,4 +14,4 @@ phases: | |
- yarn --version || npm -g install yarn | ||
build: | ||
commands: | ||
- /bin/bash ./build.sh && git diff-index --exit-code --ignore-space-at-eol --stat HEAD | ||
- /bin/bash ./build.sh --extract && git diff-index --exit-code --ignore-space-at-eol --stat HEAD | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, how many packages do we currently have rosetta extract enabled? And what is the cost of this extra work on our build time? The PR build for this PR should give you that, baselined against any other PR build. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 203 + monocdk + aws-cdk-lib = 205 have an extraction script. It doesn't look like any of the packages have strict extraction enabled currently. As I commented above, it seems to be about a 9 minute increase in build time. Not horrible but also not great. From a cursory investigation, it seems to take <50ms on my local machine to bring each worker online, so we don't seem to be losing much to overhead subprocess costs. I can do a deeper dive on how to speed it up if you think it's warranted. |
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.
We probably need to be a bit careful here.
I believe this option is required only for Node 10. In later versions of node, this has graduated out of experimental. And not supported in earlier versions of node.
What's the experience when this option is set for version > node 10? Fail or ignore?
We could be super precise and say - if node 10 and snippets extraction is set, then use
--experiemental-worker
and if < node10 and snippet extraction is set, fail?WDYT? too pedantic?
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.
The following is just speculation on how node should behave since I haven't been able to easily find node's policies on experimental features.
Providing flags that enable experimental features after the feature has stabilized should not have any effect on behaviour (confirmed for v10, v12, and v14). Further, the flag should not be reused for another purpose. Any other experience would be very frustrating to customers, forcing them to configure different versions in different ways and breaking legacy scripts.
So I don't think it's necessary to be careful with adding this flag. I'm of course happy to do so if you feel otherwise!
I also don't think we should fail if the node version is < 10, since rosetta itself still supports single-threaded extraction (though it is quite slow, especially for the monolithic modules). Rosetta already emits a warning in this case.