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

Make the node version independent of the crate version #1495

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Sep 11, 2023

No description provided.

@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Sep 11, 2023
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-additional-tests
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3647381

Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

Qs: we will version manually now? Is there a context for this change?

Overall it looks really good - now we won't get a node/worker version mismatch between releases (during development). Workers should still get rebuilt on every commit so breakages should be rare, but we just won't enforce that, which is fine for most dev. So this addresses the discussion at #626 nicely.

@@ -33,7 +33,7 @@ use tokio::{io, net::UnixStream, runtime::Runtime};
/// spawning the desired worker.
#[macro_export]
macro_rules! decl_worker_main {
($expected_command:expr, $entrypoint:expr, $worker_version:expr) => {
($expected_command:expr, $entrypoint:expr, $worker_version:expr $(,)*) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

///
/// The worker binaries associated to the node binary should ensure that they are using the same
/// version as the main node that started them.
pub const NODE_VERSION: &'static str = "1.1.0";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not env!("CARGO_PKG_VERSION")?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, okay, we don't want to use the commit hash in node version, but why the node version should be independent from the crate version in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because crates.io semver works differently to how we version our nodes.

@ordian
Copy link
Member

ordian commented Sep 12, 2023

If we're doing this, we can probably also remove rerun_if_git_head_changed in some build scripts?

Marcin> I don't think it's a good idea, we should still make a best effort to rebuild the workers on changes. They are fast to build. It's just okay not to enforce it if e.g. cargo fails to rebuild them for some reason, that should be rare, and most of the time it won't cause any problems. I think the PR as-is has a good balance for dev ux.

My reply to that is, we are separating crates releases from (polkadot) binary releases. Crates releases are mostly useful for parachain teams, which shouldn't depend on PVF crates in the first place, or at least shouldn't be affected by versioning.
That's a fair point.

@bkchr
Copy link
Member Author

bkchr commented Sep 12, 2023

Qs: we will version manually now? Is there a context for this change?

We already have done versioning manually (when we speak about the node version). The context is that crates.io semver is different to how we version our node. For now we will bump every major version of every crate for putting it onto crates.io, but the node version should not follow these bumps.

@bkchr
Copy link
Member Author

bkchr commented Sep 12, 2023

@paritytech/release-engineering you will then need to change this NODE_VERSION to change the version of the node

@bkchr bkchr merged commit e005aef into master Sep 12, 2023
117 of 120 checks passed
@bkchr bkchr deleted the bkchr-make-node-version-independent branch September 12, 2023 12:12
bkchr pushed a commit that referenced this pull request Sep 13, 2023
Add logic to check if the `BUILD_RELEASE_VERSION` tag is available in
docker registry, if not calculate the previous version to use as
secondary image. This fix the issue in test using the `secondary image`
and bumping the `NODE_VERSION`. (e.g #1495)
@bkchr bkchr mentioned this pull request Dec 7, 2023
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants