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

Pre-release naming convention #261

Open
Stargateur opened this issue May 13, 2022 · 9 comments
Open

Pre-release naming convention #261

Stargateur opened this issue May 13, 2022 · 9 comments
Labels
C-question category: a question

Comments

@Stargateur
Copy link

Stargateur commented May 13, 2022

The naming convention of this crate can be non obvious to user of SemVer 2.0. Indeed your use of pre-release tag will lead https://crates.io/crates/air-interpreter-wasm/versions order to differ than https://crates.io/crates/air-interpreter-wasm/versions?sort=semver in a way you may not expect. For example, I take only 0.24.0 pre-release:

precedence rule, what Cargo do to order them, from higher to lower:

0.24.0-update-readme.9
0.24.0-update-readme.8
0.24.0-update-readme.7
0.24.0-update-readme.6
0.24.0-update-readme.5
0.24.0-update-readme.4
0.24.0-update-readme.3
0.24.0-update-readme.2
0.24.0-update-readme.1
0.24.0-update-readme.0
0.24.0-update-faas-again.0
0.24.0-update-faas.1
0.24.0-update-faas.0
0.24.0-ttl-in-js.0
0.24.0-misc-add-developer-notes.1
0.24.0-misc-add-developer-notes.0
0.24.0-hotfix.0
0.24.0-feat-improve-scope-error-handling.0
0.24.0-feat-252-add-support-of-ttl.0

and date order more recent to more old

0.24.0-update-readme.9
0.24.0-update-readme.8
0.24.0-update-readme.7
0.24.0-update-readme.6
0.24.0-update-readme.5
0.24.0-update-readme.4
0.24.0-update-readme.3
0.24.0-update-readme.2
0.24.0-update-readme.1
0.24.0-update-readme.0
0.24.0-update-faas-again.0
0.24.0-hotfix.0
0.24.0-update-faas.1
0.24.0-update-faas.0
0.24.0-ttl-in-js.0
0.24.0-misc-add-developer-notes.1
0.24.0-misc-add-developer-notes.0
0.24.0-feat-improve-scope-error-handling.0
0.24.0-feat-252-add-support-of-ttl.0

As you can see their differ. You must be aware that most use case of pre-release will not work with your system. I would strongly advice you to not use descriptive identifier separator as first identifier of a pre-release: 0.24.0-update-readme.2 should be 0.24.0-rc.2.update_readme, 0.24.0-update-faas.0 should be 0.24.0-rc.3.update_faas, etc... notice that if you want your pre-release to be logically considerate breaking change you should do 0.24.0-hotfix.0 => 0.24.0-rc3.hotfix.0. The first identifier of a pre-release tag is often considerate as PREMAJOR like.

Thus I would still recommand to reduce your usage of pre-release, I'm not sure "update-readme" make a lot of sense for a programming user of your crate. Most other Rust crate use alpha.0, beta.0, beta.1 or rc0.0, rc1.0 or rc1.1 convention. And your version page in crates.io are already very long to load. You release a LOT of version. It's unexpected any user will be able to follow this.

@folex
Copy link
Member

folex commented May 13, 2022

@Stargateur Hi! Thank for looking so deep into this!

Let me explain why we do this, and then maybe you will find that approach feasible. Or if not, let's discuss the best approach here.

So, our goal is to be able to quickly switch to a version of a air-interpreter-wasm in projects that depend on it. For example, @mikevoronov introduces some experimental functionality to the interpreter, and the team can quickly test these changes it in the fluencelabs/fluence and fluencelabs/fluence-js projects.

update-readme, update-faas, misc-add-developer-notes, and so on are branch names. Every time there's a commit in branch, we publish a build for this branch while incrementing prerelease octet. So, we only care about order inside a single branch.

e.g. the following orders are all OK for us, since .1 always follows .0 inside a single branch:

0.24.0-update-faas.1
0.24.0-update-faas.0
0.24.0-misc-add-developer-notes.1
0.24.0-misc-add-developer-notes.0

or

0.24.0-misc-add-developer-notes.1
0.24.0-misc-add-developer-notes.0

0.24.0-update-faas.1
0.24.0-update-faas.0

or even

0.24.0-misc-add-developer-notes.1
0.24.0-update-faas.1

0.24.0-misc-add-developer-notes.0
0.24.0-update-faas.0

Does that make sense?

Is it a problem that we flood crates.io with a lot of releases? If yes, then how can we be more ecological RE rust infrastructure?

Thanks again for starting this discussion! This matters a lot.

@Stargateur
Copy link
Author

Stargateur commented May 13, 2022

Make more sense. Are you aware you can do foo = { .git = "https://github.com/whatever, branch = "my_branch" } ?

Is it a problem that we flood crates.io with a lot of releases? If yes, then how can we be more ecological RE rust infrastructure?

I have no idea what is the policy of crates.io about this. I think they may not like that. To be clear I'm not related to crates.io staff and do not speak for them.

@folex
Copy link
Member

folex commented May 13, 2022

Yeah, I know about git dependencies, but for this specific crate, we have to build it and release because it is distributed as a .wasm file.

@Stargateur
Copy link
Author

Sorry, I'm not aware of wasm technology for Rust yet, I don't understand cause crates.io will simply host source file like a git repository, so write air-interpreter-wasm = "0.24.0-misc-add-developer-notes.1" as mostly the same behavior than use directly a git repository. Can you link some doc that explain the process or explain it a little here ?

@mikevoronov mikevoronov added the C-question category: a question label May 13, 2022
@folex
Copy link
Member

folex commented May 13, 2022

Yeah sure.

So take a look at this step https://github.com/fluencelabs/aquavm/blob/master/.github/workflows/publish_interpreter.yml#L116-L118

It compiles Rust code, and produces a .wasm file. That .wasm file is basically a binary like a one you would get from usual cargo build, but in .wasm format.

We distribute that .wasm file to crates.io and npmjs.org so that fluencelabs/fluence and fluencelabs/fluence-js projects can use it and run on their built-in WASM runtimes.

It is meant to be distributed as a binary so that both cloud Fluence peer and browser Fluence peer run the very same binary.

@Stargateur
Copy link
Author

Pardon me If I said ask stupid question, do you think you could make your build.rs build the wasm instead of upload it directly ? That generally how Rust Project do. Is there a reason that explain that you didn't do that maybe ? your marine project could be used as a library in your build.rs file and build the wasm file. I don't think cargo is intended to be use to distribute a project as a binary but as a file sources distributor.

I think none restriction on the doc for crates.io rules. It's only say:

There is no limit to the number of versions which can be published

I think it's allowed to upload binary if needed like a dependence. And since there is no limit to version number you are currently fine. If you want you could reach Carol she often answer quickly on discord.

But I think if you can make build.rs build the file instead it would be better. That the first time I see a project do what you do. Fun fact I discover your project making stats on pre-release tag usage for a RFC I work and was surprise that a crates has so many unique pre-release tag.

@folex
Copy link
Member

folex commented May 13, 2022

That's a good question, actually. We've been considering such a design, but there are few considerations that drew us to the current approach:

  1. We want air-interpreter.wasm to be the same across Rust and JS runtimes. Ideally, identical. i.e., same hash
  2. Since we have JS runtime, it would still be needed to pre-build .wasm for it cuz there's no cargo in JS
  3. For Rust dependants, it would require to depend on pretty heavy stuff like marine cli
  4. build.rs would compile Rust before compiling Rust. Not sure if that would bring any complexities, but sounds complex

@Stargateur
Copy link
Author

Stargateur commented May 14, 2022

  1. that a complicated requirement indeed, You expect your tools to produce different result ?
  2. well, that I can't help, I have very few experience with JS world outside some typescript
  3. that only in build dep, not a problem at all
  4. I don't think recursive dep with a build file work, no you would only be able to build this wasm using tool that doesn't need the crate you are building since Cargo compile the thing from source. PS: I'm not sure I understand the point 4 Rust before compiling Rust

@folex
Copy link
Member

folex commented May 15, 2022

You expect tool the tool you using produce different result ?

Our tool basically does cargo build --release and then modifies binary a little bit. Since Rust compilation (cargo build) is not deterministic, the binary is different every time.

that only in build dep, not a problem at all

That's a good point, yeah.

PS: I'm not sure I understand the point 4 Rust before compiling Rust

We build air-interpreter.wasm via cargo build. So in order to build it in build.rs, we would need to run cargo build inside build.rs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-question category: a question
Projects
None yet
Development

No branches or pull requests

3 participants