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

chore: Publish near-vm-runner and it's dependencies #8782

Merged
merged 6 commits into from
Apr 17, 2023

Conversation

khorolets
Copy link
Member

My previous PR #8762 was reverted by #8780 because release automation was broken.

Looks like the cause of broken automation was a crate near-test-contract. We had a discussion about this package and the reasons to publish it in the first place in the very first PR. Now we have proof that a dependency that mentioned in the dev-dependencies section of Cargo.toml is not required to be published to crates.io.

Once figured out I created a PR #8779 but it was unnoticed and instead all the changes were reverted in #8780.

As agreed with @jakmeier I am creating the same changes but disabled near-test-contract for publishing necessary crates in the future.

khorolets and others added 2 commits March 23, 2023 13:24
…es: near-cache, near-stable-hasher) (#8762)

Co-authored-by: Vlad Frolov <frolvlad@gmail.com>
@khorolets khorolets requested a review from a team as a code owner March 23, 2023 11:30
@khorolets khorolets requested a review from nikurt March 23, 2023 11:30
Copy link
Contributor

@nikurt nikurt left a comment

Choose a reason for hiding this comment

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

Looks good but I know nothing about attaching the LICENSE files. Consider getting a second opinion.

@jakmeier
Copy link
Contributor

@andrei-near
(since you surfaced problems in this message: https://near.zulipchat.com/#narrow/stream/295302-general/topic/publishing.20nearcore.20crates/near/343928649)

Pre-commit checks are not enough to be sure we are not breaking things again, do you have a way to verify this does not lead to the same problem again?

@khorolets
Copy link
Member Author

Looks good but I know nothing about attaching the LICENSE files. Consider getting a second opinion.

I've noticed that all the packages published contain LICENSE files in their folders. Also, themis run checks for them and ensures we're using Apache 2.0 or MIT, preventing us from choosing a different license.

However, we definitely know we don't want to publish near-test-contracts but themis fails the sanity check about it

(i) These private crates break publishable crates. Either make these private crates publishable or avoid using them in the publishable crates:
--
  | ↳ runtime/near-test-contracts/Cargo.toml (depended on by near-vm-runner)
  | 🚨 Error: The command exited with status 1

@miraclx can you help us here with dev-dependencies?

Copy link
Contributor

@miraclx miraclx left a comment

Choose a reason for hiding this comment

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

Blocker resolved in #8785. One little thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's symlink to the licenses in https://github.com/near/nearcore/tree/master/licenses instead.

near-bulldozer bot pushed a commit that referenced this pull request Mar 23, 2023
Unblocks #8782, `cargo` doesn't require `dev-dependencies` to be published. There's no need for themis to have that requirement.
@khorolets khorolets requested a review from miraclx March 23, 2023 14:37
@khorolets
Copy link
Member Author

@andrei-near @jakmeier jobs are green now, thanks to @miraclx

@nagisa
Copy link
Collaborator

nagisa commented Mar 24, 2023

Can you please add a cargo publish --dry-run check to build kite as part of this PR? Without that check there's still a good deal of risk that we won't be able to publish if something is wrong, since none of the publish-time checks are being run right now...

cc #8781

@khorolets
Copy link
Member Author

khorolets commented Mar 24, 2023

Can you please add a cargo publish --dry-run check to build kite as part of this PR? Without that check there's still a good deal of risk that we won't be able to publish if something is wrong, since none of the publish-time checks are being run right now...

cc #8781

The naive approach of running cargo publish --dry-run in the root of the repo doesn't work (see the terminal output)

$ cargo publish --dry-run                                                                                                                                                   
error: the `-p` argument must be specified to select a single package to publish

I presume the publishing job is defined somewhere else I don't know where or don't have access.
Could you please tag someone who might know and can handle it, since unfortunately, I cannot?

@nagisa
Copy link
Collaborator

nagisa commented Mar 24, 2023

I believe https://github.com/near/near-ops/blob/a7dbb2cc47addaf3cb294e07c269964c8051c41c/buildkite/pipelines/nearcore-release.yml#L37-L55 is the part that's responsible for publishing the crates. Unfortunately that's not particularly helpful here, as cargo ws publish does not support --dry-run. But since we do use a fork of it for publishing already, we can also look into adding --dry-run for it, I guess.

Either way, this is more complicated than I initially anticipated, so I'm not going to block this change on that…

@jakmeier
Copy link
Contributor

In that case, coming back to @andrei-near : Do you want to check if the canary build fails again, which you pointed out for the now reverted PR in Zulip, or do you think we can merge this as is?

@Ekleog-NEAR
Copy link
Collaborator

I tried thinking a bit about it, but having a cargo ws publish --dry-run would be highly nontrivial, as one crate would depend on another in order to be published, so it could not rely on cargo publish --dry-run at all. I think the best that could be done might be using another rust target folder and checking that cargo test passes for each crate with publish = true, plus verifying that crates with publish = true can only depend on crates with publish = true… but as @nagisa says, it would be hard to actually do this.

TBH I’m not sure the current "bump the version in cargo.toml and CI will publish the changes" scheme is the best one, as CI failures are silent in this case, but I guess anything better would be hard to switch to.

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

It seems there are no simple ways to fix this for now and @andrei-near hasn't raised any concerns with merging this, so should we just merge this and check all post-merge CI jobs?

And then while working on limited-replayability, maybe @Ekleog-NEAR you can think more about how the ideal process of publishing would look like? I'd say we can circle back to improve the process once we know how limited-replayability is implemented.

near-bulldozer bot pushed a commit that referenced this pull request Apr 3, 2023
This is needed for #8782 to go well with the changes incoming with landing near-vm.

I have literally no idea how to test this, so I tried copying the way of doing things of #8782 and hope it’s right.
nikurt pushed a commit to nikurt/nearcore that referenced this pull request Apr 5, 2023
This is needed for near#8782 to go well with the changes incoming with landing near-vm.

I have literally no idea how to test this, so I tried copying the way of doing things of near#8782 and hope it’s right.
@near-bulldozer near-bulldozer bot merged commit e59885a into master Apr 17, 2023
@near-bulldozer near-bulldozer bot deleted the chore/publish-near-vm-runner branch April 17, 2023 18:47
@Ekleog-NEAR
Copy link
Collaborator

Uhh just noticed this, thank you for merging and sorry for not having noticed I had forgotten to set the automerge label!

nikurt pushed a commit that referenced this pull request Apr 18, 2023
My previous PR #8762 was reverted by #8780 because release automation was broken.

Looks like the cause of broken automation was a crate `near-test-contract`. We had a discussion about this package and the reasons to publish it in the first place in the very first PR. Now we have proof that a dependency that mentioned in the `dev-dependencies` section of `Cargo.toml` is not required to be published to crates.io.

Once figured out I created a PR #8779 but it was unnoticed and instead all the changes were reverted in #8780.

As agreed with @jakmeier I am creating the same changes but disabled `near-test-contract` for publishing necessary crates in the future.
nikurt pushed a commit that referenced this pull request Apr 25, 2023
My previous PR #8762 was reverted by #8780 because release automation was broken.

Looks like the cause of broken automation was a crate `near-test-contract`. We had a discussion about this package and the reasons to publish it in the first place in the very first PR. Now we have proof that a dependency that mentioned in the `dev-dependencies` section of `Cargo.toml` is not required to be published to crates.io.

Once figured out I created a PR #8779 but it was unnoticed and instead all the changes were reverted in #8780.

As agreed with @jakmeier I am creating the same changes but disabled `near-test-contract` for publishing necessary crates in the future.
nikurt pushed a commit that referenced this pull request Apr 28, 2023
This is needed for #8782 to go well with the changes incoming with landing near-vm.

I have literally no idea how to test this, so I tried copying the way of doing things of #8782 and hope it’s right.
nikurt pushed a commit that referenced this pull request Apr 28, 2023
My previous PR #8762 was reverted by #8780 because release automation was broken.

Looks like the cause of broken automation was a crate `near-test-contract`. We had a discussion about this package and the reasons to publish it in the first place in the very first PR. Now we have proof that a dependency that mentioned in the `dev-dependencies` section of `Cargo.toml` is not required to be published to crates.io.

Once figured out I created a PR #8779 but it was unnoticed and instead all the changes were reverted in #8780.

As agreed with @jakmeier I am creating the same changes but disabled `near-test-contract` for publishing necessary crates in the future.
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.

6 participants