-
Notifications
You must be signed in to change notification settings - Fork 38
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
Native compilation #1700
Native compilation #1700
Conversation
What is the feature?Does it have an issue?No. It's just a sub point of #1691 What is it doing?Explanation
QuestionsWhat on Earth is native compilation? It looks like it is compiling the old way without podman/docker. I am guessing ExpectationsTestsI think I would expect the tests to be the same. I would just expect as the CodeI would expect the code the just have an if statement based on the flag and There might be better error handling, but I am guessing that would be a fair I would expect it to look like what we had before podman. This is from a bit of spoilers that I saw while I was trying to figure out what |
Old Versionimport { execSync, IOType } from 'child_process';
import {
GLOBAL_AZLE_RUST_DIR,
GLOBAL_AZLE_RUST_BIN_DIR,
GLOBAL_AZLE_TARGET_DIR,
time
} from './utils';
export function compileRustCode(
canisterName: string,
canisterPath: string,
stdio: IOType
) {
execSync(
`cd ${canisterPath} && ${GLOBAL_AZLE_RUST_BIN_DIR}/cargo build --target wasm32-wasi --manifest-path canister/Cargo.toml --release`,
{
stdio,
env: {
...process.env,
CARGO_TARGET_DIR: GLOBAL_AZLE_TARGET_DIR,
CARGO_HOME: GLOBAL_AZLE_RUST_DIR,
RUSTUP_HOME: GLOBAL_AZLE_RUST_DIR,
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: 'sparse'
// TODO this allows changing the stack size, could be useful for stack overflow or heap out of bounds errors
// RUSTFLAGS: '-C link-args=-zstack-size=2000000000'
}
}
);
const wasmTargetFilePath = `${GLOBAL_AZLE_TARGET_DIR}/wasm32-wasi/release/canister.wasm`;
execSync(`cp ${wasmTargetFilePath} ${canisterPath}`);
execSync(
`cd ${canisterPath} && ${GLOBAL_AZLE_RUST_BIN_DIR}/wasi2ic canister.wasm ${canisterName}.wasm`,
{
stdio,
env: {
...process.env,
CARGO_TARGET_DIR: GLOBAL_AZLE_TARGET_DIR,
CARGO_HOME: GLOBAL_AZLE_RUST_DIR,
RUSTUP_HOME: GLOBAL_AZLE_RUST_DIR
}
}
);
} So in this old version we have three commands we are running
There is a bunch more stuff to figure out all the right paths and file names but |
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.
Results
There are a bunch of files that added target back to the .gitignore and added
the --native-compilation flag to the dfx.json. I don't think those files need to
be reviewed much.
test.ymal adds back all of the dependencies for native installation. It doesn't
look like it cares if the node is going to be running a native compilation test
or a podman test. Is it even possible to determine that before the test is running?
That leaves the meat of the review to pass through the rubric.
Rubric
- Consistency (Same as the rest of the code and itself)
- No new files, new variable (
nativeCompilation
) looks good. New
functions (compileRustCodeNatively
andcompileRustCodeWithPodman
) look
good. There isn't much more to look at here. It's just a couple of if
statements and passing variables through.
- No new files, new variable (
- Readability (Easy to understand)
- As far as readability goes I am finding it a bit confusing that we still
use things based off of the dockerContainerName or dockerHash even if we are
compiling natively. like here or here.
I get that it's nice to have one function for compileRustCode and then it
determines which way to compile it, but I would find it weird if I ever, for
example, called it in a context where we were only compiling natively, and we
don't even have a dockerContainerName. Realistically that will probably
never happen...
- As far as readability goes I am finding it a bit confusing that we still
- Maintainability (modular, reusable, and simple)
- Looks good, not much has been added that would affect this, both
compileRustCodeWithPodman
andcompileRustCodeNatively
are encapsulated
in their own functions, that's good.
- Looks good, not much has been added that would affect this, both
- Correctness (as bug free as reasonable)
- I think the only things that pertain to this category are more
appropriately covered in the following.
- I think the only things that pertain to this category are more
- Code level implications (code coverage, error coverage, conditionals coverage, efficiency, etc)
- I guess
https://github.com/demergent-labs/azle/pull/1700/files#diff-bc0edaf897df65b29139d6cc26dfacd8dd366af89f38b99e6e22c672df55cfe6R72
would raise the question of if there are other things we ought to ignore if
we are doing native compilation. Which one of us has the burden to prove
that it's just preparing the image and then the actual compilation? - This looks like a different compilation process than before?
- Expected
cd <canister-path> && cargo build <with target, manifest, release, and env> cp <wasm> <canister> cd <canister-path> && wasi2ic canister.wasm <cansiterName>.wasm <with env>
- Actual
We aren'tcargo build <with target, manifest, release> wasi2ic canister.wasm <cansiterName>.wasm
cd
ing orcp
ing any more... what's up with that?
- I guess
- Quality of tests and documentation (thinking about how others will use it)
- No updates to the tests besides what I was expecting, (a few that
have the addition of the native-compilation flag and that's it) - Where would documentation for this feature be?
- We should at least add this flag to
azle --help
We should add
azle --help
. Could you make an issue to createazle --help
that will
explain all of our flags and usage to developers?
- We should at least add this flag to
- No updates to the tests besides what I was expecting, (a few that
Code
Code is more or less as I was expecting, no big changes need.
Tests
Tests are mostly unchanged with just a few changing their compilation method,
just as expected.
src/compiler/compile_rust_code_with_candid_and_compiler_info.ts
Outdated
Show resolved
Hide resolved
The merge-base changed after approval.
The merge-base changed after approval.
No description provided.