Skip to content
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 missing features #10

Merged
merged 8 commits into from
Aug 8, 2024
Merged

Add missing features #10

merged 8 commits into from
Aug 8, 2024

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Aug 7, 2024

Somehow the command from the README does not build for me, going to put in the CI to see if it reproduces.

ggwpez added 3 commits August 7, 2024 22:27
Somehow the command from the README does not build for me, going to put in the CI to see if it reproduces.
@rzadp
Copy link
Contributor

rzadp commented Aug 8, 2024

The CI has run out of disk space on this run.

But it looks like the command has passed successfully before that happened:

(also both work for me locally)

@ggwpez Is cargo build working for you, or is it just the --release one? Do you perhaps see similar errors as here?

@ggwpez
Copy link
Member Author

ggwpez commented Aug 8, 2024

Yes it works now because i added the features. You can see the previous run here: https://github.com/paritytech/polkadot-sdk-parachain-template/actions/runs/10291282486/job/28483191721

So adding the features should fix it either way.

@rzadp
Copy link
Contributor

rzadp commented Aug 8, 2024

Yes it works now because i added the features.

Right, right, makes sense.
Running cargo build --package parachain-template-node --release without those fixes fails for me as well.

BUT - running cargo build --release on master is passing for me without any issues.
This is what I was running because I thought this built all the packages - including parachain-template-node.
Also tried cargo build --release --all.

Not sure what to do about next in terms of CI - do we have to check 6 variants in each template (3 packages, dev and release)?
This is very counterintuitive for me, how can building all packages pass but building one fails?

@rzadp
Copy link
Contributor

rzadp commented Aug 8, 2024

Anyways I added analogical PRs to the other templates, and will add a PR to polkadot-sdk after that as well.

But it's still not clear to me what happens here, probably missing experience with how Rust features work.

@ggwpez
Copy link
Member Author

ggwpez commented Aug 8, 2024

This is very counterintuitive for me, how can building all packages pass but building one fails?

It is how cargo determines what features to enable. If you only build a single package then it may not enable all features since it only looks at that crate - not the whole workspace.
I would suggest to add the std feature like i did here in the merge request to make it build even for a single crate. In the polkadot SDK we have a CI to check for this but its overkill here.

Additionally you can also change the build instructions in the readme just to carbo build --release since it is shorter.

Not sure what to do about next in terms of CI - do we have to check 6 variants in each template (3 packages, dev and release)?

You can change all to cargo build --release, i think the build time is not much longer since the repos contain no clutter?

@rzadp
Copy link
Contributor

rzadp commented Aug 8, 2024

Sounds good, I think I'm getting it now.
I'll merge the PRs so that the templates are repaired, and will send a PR to polkadot-sdk with final changes.

@rzadp rzadp changed the title [CI] Test README example Add missing features Aug 8, 2024
@shawntabrizi
Copy link
Member

shawntabrizi commented Aug 8, 2024

Running cargo build --package parachain-template-node --release without those fixes fails for me as well.

BUT - running cargo build --release on master is passing for me without any issues.

Right, but this was the command that was included in the readme.... so it really should have been the one that was tested, or you should have put in the readme the steps you did which actually passed.

@rzadp rzadp merged commit 58f58b2 into master Aug 8, 2024
4 checks passed
@rzadp rzadp deleted the oty-ci-readme-example branch August 8, 2024 16:40
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Aug 9, 2024
Corrects the issue we had
[here](paritytech/polkadot-sdk-parachain-template#10),
in which `cargo build --release` worked but `cargo build --package
parachain-template-node --release` failed with missing features.

The command has been added to CI to make sure it works, but at the same
we're changing it in the readme to just `cargo build --release` for
simplification.

Labeling silent because those packages are un-published as part of the
regular release process.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Aug 28, 2024
Corrects the issue we had
[here](paritytech/polkadot-sdk-parachain-template#10),
in which `cargo build --release` worked but `cargo build --package
parachain-template-node --release` failed with missing features.

The command has been added to CI to make sure it works, but at the same
we're changing it in the readme to just `cargo build --release` for
simplification.

Labeling silent because those packages are un-published as part of the
regular release process.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
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.

3 participants