Skip to content

Conversation

FrancescoV1985
Copy link

-Documented in CONTRIBUTING.md the role of sysroot and the meaning of different flags combination used in frontend script y.sh
-Updated help output for binary file y (build_system/target/release/y build --help)

@FrancescoV1985
Copy link
Author

Fixes #646

Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay.
Here's a first review.
Thanks for your work!

Rustup ships these libraries **pre-compiled with LLVM**.

Because **rustc_codegen_gcc** replaces LLVM with the GCC backend, the shipped
LLVM artefacts are **incompatible**.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should be compatible. If they are not, this would be an ABI bug.

* the user switches toolchains / updates submodules.

Both backend and sysroot can be built using different [profiles](https://doc.rust-lang.org/cargo/reference/profiles.html#default-profiles).
That is exactly what the `--sysroot`, `--release-sysroot` and `--release` flag supported by the frontend script `y.sh` take care of.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
That is exactly what the `--sysroot`, `--release-sysroot` and `--release` flag supported by the frontend script `y.sh` take care of.
That is exactly what the `--sysroot`, `--release-sysroot` and `--release` flag supported by the build system script `y.sh` take care of.


| Command | Backend Profile | Sysroot Profile | Usage Scenario |
|--------------------------------------------|-------------------------------|----------------------------------|------------------------------------------------------------|
| `./y.sh build` |  dev (optimized + debuginfo) |  X |  Optimize backend with debug capabilities |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the Cargo.toml, only the dependencies are built with optimizations.

|--------------------------------------------|-------------------------------|----------------------------------|------------------------------------------------------------|
| `./y.sh build` |  dev (optimized + debuginfo) |  X |  Optimize backend with debug capabilities |
| `./y.sh build --release` |  release (optimized) |  X |  Optimize backend without rebuilding sysroot |
| `./y.sh build --release --release-sysroot`|  release (optimized) |  X |  Same as --release |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--release-sysroot should not be used without --sysroot, so I would remove this line and the other doing this from this table and perhaps specify that this outside the table.

| `./y.sh build` |  dev (optimized + debuginfo) |  X |  Optimize backend with debug capabilities |
| `./y.sh build --release` |  release (optimized) |  X |  Optimize backend without rebuilding sysroot |
| `./y.sh build --release --release-sysroot`|  release (optimized) |  X |  Same as --release |
| `./y.sh build --release --sysroot` |  release (optimized) |  dev (unoptimized + debuginfo) |  Optimize backend for release without full sysroot rebuild |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should build the sysroot since the --sysroot flag is provided.

| `./y.sh build --release` |  release (optimized) |  X |  Optimize backend without rebuilding sysroot |
| `./y.sh build --release --release-sysroot`|  release (optimized) |  X |  Same as --release |
| `./y.sh build --release --sysroot` |  release (optimized) |  dev (unoptimized + debuginfo) |  Optimize backend for release without full sysroot rebuild |
| `./y.sh build --sysroot` |  dev (optimized + debuginfo) |  dev (unoptimized + debuginfo) |  Full debug capabilities with optimized backend |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should build the sysroot since the --sysroot flag is provided.

| `./y.sh build --release --sysroot` |  release (optimized) |  dev (unoptimized + debuginfo) |  Optimize backend for release without full sysroot rebuild |
| `./y.sh build --sysroot` |  dev (optimized + debuginfo) |  dev (unoptimized + debuginfo) |  Full debug capabilities with optimized backend |
| `./y.sh build --release-sysroot` |  dev (optimized + debuginfo) |  X |  Build only optimized backend with debug capabilities |
| `./y.sh build --release-sysroot --sysroot`|  dev (optimized + debuginfo) |  release (optimized) |  Build optimized backend and sysroot for debugging and release, respectively |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only build a release sysroot, not a debug sysroot.
And this builds a "debug" (well, dependencies compiled with optimizations + backend not optimized) backend.

Comment on lines +480 to +490
--release : Build and optimize the backend in release mode
--sysroot : When used on its own, build both
sysroot and backend in dev. mode. Only the backend is optimized.
When used together with --release, build and optimize the backend
in release mode instead of in dev. mode.
When used together with --release-sysroot,
build and optimize the sysroot in release mode instead of in dev. mode
--release-sysroot : When used on its own, build and optimize only the backend in dev. mode.
When combined with --release, it has no effect.
When combined with --sysroot, additionally
build and optimize the sysroot in release mode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let you update this to reflect the above comments.
Also, most of these flags will only work with the build command, right?
If so, they should only be shown in the help of the build command.

@FrancescoV1985
Copy link
Author

Sorry for the delay. Here's a first review. Thanks for your work!

Good points :), thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants