-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add workspace optimizer #23
Conversation
Open problems:
|
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.
Nice work. I will finish it.
# Install Rust nightly | ||
# Choose version from: https://rust-lang.github.io/rustup-components-history/x86_64-unknown-linux-gnu.html | ||
RUN rustup toolchain install nightly-2020-08-20 --allow-downgrade --profile minimal --target wasm32-unknown-unknown | ||
RUN rustup default nightly-2020-08-20 |
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.
Okay, nightly rust for determinstic compilation seems odd to me. I guess you are pinning it to nightly-2020-08-20
not just any nightly, but still feels odd.
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. Not optimal but I also don't see an issue with this. Unfortunately there is not versioned nightly tag on docker directly, so we have to install it manually.
all_packages.sort() | ||
log("Package directories:", all_packages) | ||
|
||
contract_packages = [p for p in all_packages if p.startswith(PACKAGE_PREFIX)] |
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.
This seems quite complex way to do contracts/*
"Get all and take those that start with contracts/
" is basically that, but none if contracts are not in members. Can't we just assume that
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 see this as a transition from searching Cargo.toml
s to parsing and understanding the workspace. The current path filter is probably not ideal. Maybe we end up with a way to filter contracts from non-contracts using a more stable way.
But this is already an improvement over path search since you can specify members = ["contracts/a", "contracts/b", "packages/*"]
and then contracts/c
will not be built since it is not in the workspace.
log("Optimizing built {} ...".format(build_result)) | ||
name = os.path.basename(build_result) | ||
cmd = ["wasm-opt", "-Os", "-o", "artifacts/{}".format(name), build_result] | ||
subprocess.check_call(cmd) |
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 can remove the ./contracts_artifacts
dir after this loop, right?
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.
Right, that should do the job. Even better would be to write the output to a place outside of /code
, such that you never need to touch the host filesystem for this step. Ideally using
import tempfile
# ...
for contract in contract_packages:
log("Building {} ...".format(contract))
with tempfile.TemporaryDirectory() as out_dir:
# Rust nightly and unstable-options is needed to use --out-dir
cmd = [CARGO_PATH, "-Z=unstable-options", "build", "--release", "--target=wasm32-unknown-unknown", "--locked", "--out-dir={}".format(out_dir)]
os.environ["RUSTFLAGS"] = "-C link-arg=-s"
subprocess.check_call(cmd, cwd=contract)
for build_result in glob.glob(out_dir + "/*.wasm"):
log("Optimizing built {} ...".format(build_result))
name = os.path.basename(build_result)
cmd = ["wasm-opt", "-Os", "-o", "artifacts/{}".format(name), build_result]
subprocess.check_call(cmd)
(untested)
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.
Done without the with (no need to clean up the temp dir on docker).
But result same: not touching host filesystem for out_dir
I think this is done now. I will merge and publish shortly. |
Misses a lot of documentation but should be basically feature complete.