feat: production build #354
Conversation
| use super::*; | ||
| use cli::MockCli; | ||
| use strum::VariantArray; | ||
|
|
||
| fn add_production_profile(project: &Path) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Happy to hear where I can place this function as it now exists in three places.
There was a problem hiding this comment.
I had the same question in the pop call contract. The solution was to have a public function in the crate pop_contracts under a testing module that can be then used by the pop-cli https://github.com/r0gue-io/pop-cli/pull/306/files#diff-1735e49d7426a2bb8a539485425778554b43ef1ca455b26bd8138b69c69b18aeR27
There was a problem hiding this comment.
If we see it can be used in contracts then Ale's suggestion is the best approach.
There was a problem hiding this comment.
It will be used for parachains and builds, not for smart contracts as far as I'm aware. However, I see no reason not to place it in common.
@AlexD10S your suggestion doesn't use any feature flag? This means it is added to the compilation time of the users which doesn't seem ideal. I suggest we come up with a feature name that we can start to use in the repository
There was a problem hiding this comment.
A random idea; we add it to the templates Cargo.toml files which makes it a public crate function in general. Adding it will make the pop cli users aware of this possibility and will improve their chain when they go live to production, through this we create awareness for builders, a win.
There was a problem hiding this comment.
Looking at what it is actually doing, https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-common/src/manifest.rs might be the best candidate. It arguably should be improved to use the functionality available via the Manifest object - e.g. reading the manifest, checking if the profile already exists and adding if not.
Note: if it is moved to a library crate it should not use anyhow: https://github.com/dtolnay/anyhow?tab=readme-ov-file#comparison-to-thiserror.
There was a problem hiding this comment.
That is a great suggestion, I will not add it to the templates as default, something we can consider in the future
Daanvdplas
requested review from
AlexD10S and
al3mart
and removed request for
AlexD10S
| pub(crate) release: bool, | ||
| /// Build profile [default: debug]. | ||
| #[clap(long, value_enum)] | ||
| pub(crate) profile: Option<Profile>, |
There was a problem hiding this comment.
https://github.com/r0gue-io/pop-cli/blob/fix/build-spec/crates/pop-cli/tests/parachain.rs#L13
Should be updated accordingly to this change.
| @@ -40,20 +38,20 @@ pub(crate) enum ChainType { | |||
| // A development chain that runs mainly on one node. | |||
| #[default] | |||
| #[strum( | |||
| serialize = "Development", | |||
| serialize = "development", | |||
There was a problem hiding this comment.
Needs the capitalization. Otherwise the command seems to complete successfully.
There was a problem hiding this comment.
I don't understand. I changed this so that the user doesn't have to write "Development" but "development"
There was a problem hiding this comment.
Yeah, so I'm not really familiar with the details of strum here.
But the reason of the failure is that the the node or spec builder requires this to be "Development". So the pain point is when this variant is serialized into the spec.
Although, even if we have serialize = "Development" users will be able to use --type development just fine. It's a bit confusing.
There was a problem hiding this comment.
Isnt there an ignore case flag on strum? https://docs.rs/strum/latest/strum/additional_attributes/index.html
| message = "Development", | ||
| detailed_message = "Meant for a development chain that runs mainly on one node." | ||
| )] | ||
| Development, | ||
| // A local chain that runs locally on multiple nodes for testing purposes. | ||
| #[strum( | ||
| serialize = "Local", | ||
| serialize = "local", |
| message = "Local", | ||
| detailed_message = "Meant for a local chain that runs locally on multiple nodes for testing purposes." | ||
| )] | ||
| Local, | ||
| // A live chain. | ||
| #[strum(serialize = "Live", message = "Live", detailed_message = "Meant for a live chain.")] | ||
| #[strum(serialize = "live", message = "Live", detailed_message = "Meant for a live chain.")] |
| @@ -28,9 +29,9 @@ pub(crate) struct BuildArgs { | |||
| /// The package to be built. | |||
| #[arg(short = 'p', long)] | |||
| pub(crate) package: Option<String>, | |||
| /// For production, always build in release mode to exclude debug features. | |||
| #[clap(short, long)] | |||
| pub(crate) release: bool, | |||
There was a problem hiding this comment.
This is a breaking change.
Users that were running with pop build --release won't be able to just use this newer version. Imagine scripts.
We should consider a deprecation message and maybe an alias from --release to --profile=release during at least one more release of the CLI.
There was a problem hiding this comment.
An alias is ideal - the build command should have some familarity with cargo, allowing -r, --release or --profile release
There was a problem hiding this comment.
It should not be deprecated in my view.
There was a problem hiding this comment.
Not a bad idea to use the alias.
A suggestion would be to add the conflicts_with rule to prevent users from using both the --release and --profile=production flags at the same time.
#[clap(short, long, conflicts_with="profile")]
pub(crate) release: bool,
There was a problem hiding this comment.
I agree that it should not be deprecated for the normal build command and put it back in
There was a problem hiding this comment.
The only change requested is based on Ale's feedback about keeping the --release flag as deprecated.
Here’s how you can handle it:
1- Update the help message for the flag: https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-cli/src/commands/test/contract.rs#L18
2- If the flag is used, display a warning to inform the user: https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-cli/src/commands/test/contract.rs#L50
3- You can set the Profile to release if the flag was specified.
|
I'll do the rebase here |
al3mart
force-pushed
the
daan/feat-production_build
branch
from
d5d5bc0
to
7e46a67
Compare
chore: feedback and rebase
| // All commands originating from root command are valid | ||
| BuildParachainCommand { | ||
| path: args.path, | ||
| package: args.package, | ||
| release: args.release, | ||
| profile: Some(profile), |
There was a problem hiding this comment.
The Option requirement should be removed when removing the deprecated parachain build and contract command.
Adds the build profile to pop cli