-
Notifications
You must be signed in to change notification settings - Fork 283
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
feat: Build system handles dynamic deps first class. #2283
Conversation
… 1 line of jq to get deps for ts.
>&2 echo "Missing rebuildPatterns property. Either filename as string, or patterns as array." | ||
function get_deps { | ||
local TYPE=$(jq -r ".\"$1\".dependencies | type" $MANIFEST) | ||
if [ "$TYPE" == "string" ]; then |
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.
Thoughts on this logic being in JS in the future?
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'd ask, 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.
More scrutable scripting language for most our devs - jq is its own DSL, basically
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.
Meh. Work for works sake. With chatgpt this took me 5 minutes and 1 line of jq. Glorious!
|
||
cd "$(dirname "$0")" | ||
|
||
echo yarn-project-base |
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.
Similarly wonder what you think about a future move to JS
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.
Meh. Work for works sake. With chatgpt this took me 5 minutes and 1 line of jq. Glorious!
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.
But to be less dismissive, and for those reading up on this PR later, the downsides of this kind of approach is that is can slow down a bit if you go to far with processes calling processes calling processes...
I ran into that earlier with some of the dependency calculation stuff, but resolved it with an assoc array and early outs... an algorithm a sensible person would have done in another language anyway.
But yeah, if at some point this becomes unfit for purpose then can reconsider.
If this kind of code is "ballooning", my thought would be why are we adding so much devops stuff!? Rather than do we need to start changing how we do devops stuff.
Less is more, KISS, etc etc
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 woke up with a bit more to say on this.
First r.e. performance, it already takes ~3s to compute the rebuild patterns for e.g. end-to-end, which has a larger graph. If this were to double or more, one might question doing it in e.g. node.
However, then you need to install node in our tiny build image, so we need to balance that tradeoff in additional download time, or installing nodejs in the alpine container at runtime. A quick test on mainframe makes the later seem near instant, but mainframe has super fast stable network (cci questionable).
Installing nodejs grows the uncompressed image from 188MB to 239MB, however the existing build image is only 53MB compressed, so extending that compression ratio we could expect the image data download to increase to 67MB. This is perhaps negligible, but it's also just a quick estimate.
build-system/scripts/query_manifest
Outdated
declare -A ALL_DEPS | ||
add_deps() { | ||
if [[ -v ALL_DEPS[$1] ]]; then | ||
return | ||
fi | ||
ALL_DEPS["$1"]=1 | ||
DEPS=($(jq -r ".\"$1\".dependencies // [] | .[]" $MANIFEST)) | ||
get_deps $1 | ||
# DEPS=($(jq -r ".\"$1\".dependencies // [] | .[]" $MANIFEST)) |
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.
Commented out line
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.
LGTM
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.7.1</summary> ## [0.7.1](aztec-packages-v0.7.0...aztec-packages-v0.7.1) (2023-09-14) ### Features * Build system handles dynamic deps first class. ([#2283](#2283)) ([f66077a](f66077a)) * Build_manifest default tweaks. ([#2287](#2287)) ([c8a5cfb](c8a5cfb)) * **build:** Build multi-architecture docker images for aztec-sandbox ([#2305](#2305)) ([8ee61b8](8ee61b8)) * Cli "unbox" command ([#2029](#2029)) ([26ab88f](26ab88f)) * Creating an SMT verification module ([#1932](#1932)) ([4642b61](4642b61)) * Token standard ([#2069](#2069)) ([5e8fbf2](5e8fbf2)) ### Bug Fixes * Ensure_note_hash_exists ([#2256](#2256)) ([271b060](271b060)) * Msgpack stack blowups on schema gen ([#2259](#2259)) ([1afc566](1afc566)) * Noir bootstrap ([#2274](#2274)) ([f85db49](f85db49)) * Workaround sequencer timeout ([#2269](#2269)) ([9fc3f3d](9fc3f3d)) ### Miscellaneous * Bump nargo to 0.11.1-aztec.0 ([#2298](#2298)) ([8b76a12](8b76a12)) * **ci:** Mirror Aztec-nr ([#2270](#2270)) ([c57f027](c57f027)) * **circuits:** Base rollup cbind msgpack ([#2263](#2263)) ([0d4c707](0d4c707)) * **circuits:** Clean up of some superfluous header includes ([#2302](#2302)) ([5e53345](5e53345)) * **circuits:** Removing assertMemberLength on Tuple objects ([#2296](#2296)) ([0247b85](0247b85)) * Consolidate mirror repos on a nightly schedule ([#1994](#1994)) ([1a586c4](1a586c4)) * **docs:** Rename to aztec.nr ([#1943](#1943)) ([a91db48](a91db48)) * Move barretenberg to top of repo. Make circuits build off barretenberg build. ([#2221](#2221)) ([404ec34](404ec34)) * Replace native token in lending contract ([#2276](#2276)) ([c46b3c8](c46b3c8)) * **subrepo:** Push aztec-nr, update default branches ([#2300](#2300)) ([80c9b77](80c9b77)) * Updated `acvm_js` ([#2272](#2272)) ([9f1a3a5](9f1a3a5)) </details> <details><summary>barretenberg.js: 0.7.1</summary> ## [0.7.1](barretenberg.js-v0.7.0...barretenberg.js-v0.7.1) (2023-09-14) ### Miscellaneous * Move barretenberg to top of repo. Make circuits build off barretenberg build. ([#2221](#2221)) ([404ec34](404ec34)) </details> <details><summary>barretenberg: 0.7.1</summary> ## [0.7.1](barretenberg-v0.7.0...barretenberg-v0.7.1) (2023-09-14) ### Miscellaneous * Move barretenberg to top of repo. Make circuits build off barretenberg build. ([#2221](#2221)) ([404ec34](404ec34)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
If no deps provided, query-manifest exectutes script in project, uses 1 line of jq to get deps for ts.