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

[Docs] Add some Rust docs #14394

Closed
wants to merge 2 commits into from
Closed

[Docs] Add some Rust docs #14394

wants to merge 2 commits into from

Conversation

kripken
Copy link
Member

@kripken kripken commented Jun 7, 2021

No description provided.

@kripken kripken requested review from RReverser and tlively June 7, 2021 16:57
@kripken kripken changed the title [Rust] Add some Rust docs [Docs] Add some Rust docs Jun 7, 2021
Comment on lines +37 to +38
emsdk install 56b877afe7d1b651d6b9a2ba5d5df074876172ff
emsdk activate 56b877afe7d1b651d6b9a2ba5d5df074876172ff
Copy link
Member

Choose a reason for hiding this comment

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

It would be worth finding and documenting the first tagged Emscripten release after this point, IMO, rather than suggesting that users install an untagged release. A priori, I would expect that the risk of LLVM incompatibilities would be outweighed by the downsides of documenting an untagged git commit, namely that the commit is impossible to remember and raises questions about stability for users who do not know about our release process.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the hash of a roll, so it has passed the roller's full test suite. For that reason I don't think the risk is significantly higher than a release.

It worries me to allow several further rolls in between (which is what going to the next release would cause). It's not just LLVM incompatibilities, it's binaryen as well, where LLVM and binaryen overlap (say, on the binary format of a new feature).

I think the minimal possible distance in terms of commits, that is still fully tested, is safest.

Copy link
Collaborator

@RReverser RReverser Jun 7, 2021

Choose a reason for hiding this comment

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

As far as I understand, we can add arbitrary tags to hashes in emsdk pretty easily. Could we add a tag for this version like rust-1.52.1 or llvm-12 to make it easier to use & remember? At least until we automate this, it seems useful to have human-readable names.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Yes, it is as simple as that.

+1 for adding both of those tags there, as both could be useful for people wanting to use emscripten with a provided LLVM.

We should also test them. Testing the LLVM one can be done with a PR in emscripten (that replaces the tot build with LLVM 12). For Rust, I'm not sure how best to do it, but one option might be to create an emscripten-core/rust repo and add some amount of testing there. Or perhaps the repo could be more generically named, as we may want multiple languages there eventually (e.g. Zig).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it is as simple as that.

(narrator voice) It wasn't as simple as that 😅 emscripten-core/emsdk#836

Comment on lines +40 to +44
(For more on how to find the proper Emscripten hash for a particular LLVM
commit, see our
`release process docs <https://github.com/emscripten-core/emscripten/blob/main/docs/process.md#release-processes>`_
which are summarized in the
:ref:`developer's guide <developers-guide-bisecting>`.)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be recommending these docs to end users, even parenthetically. We should make using Rust with Emscripten appear as simple as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be happy to remove it to a lower section "for advanced users" or such? I do think there is value for the type of developer that does want to know how things work internally.

RReverser added a commit to RReverser/emsdk that referenced this pull request Jun 8, 2021
These are human-readable tags for emscripten-core/emscripten#14394 (comment).

I went with `rust-compat` instead of `rust` because `emsdk install rust` might be confusing and suggest that it will install Rust itself too.
@kripken
Copy link
Member Author

kripken commented Jun 8, 2021

Btw, checking this again, I'm not sure we did it right. The LLVM change here is when trunk was switched to report itself as 12 - which was a year ago. Is that what Rust uses currently? (Or does Rust use one of the release candidates for LLVM 12, which would be many months later?)

@RReverser
Copy link
Collaborator

@kripken As far as I can tell, Rust currently uses LLVM 12 build from April:

https://github.com/rust-lang/rust/blob/c4b540698165f2172dac8562bde84116f65d13bb/.gitmodules#L37

Because there are some custom patches involved, it's a bit hard to tell which specific upstream LLVM hash it corresponds to, but the fork with list of commits is here: https://github.com/rust-lang/llvm-project/commits/rustc/12.0-2021-04-15

@kripken
Copy link
Member Author

kripken commented Jun 9, 2021

April 2021? That's very different than the LLVM revision @tlively pointed to, which was from July 2020 (almost a year ago), https://github.com/llvm/llvm-project/releases/tag/llvmorg-12-init So seems like we need to figure this out...

It's very hard for me to see what's going on in those last links. April 15 2021 is in the branch name, but there is nothing special about that date in the history - it has commits both before and after that date that do not seem to correspond to upstream LLVM (or that are cherry-picks from it).

@RReverser
Copy link
Collaborator

it has commits both before and after that date that do not seem to correspond to upstream LLVM (or that are cherry-picks from it)

Yeah I think they cherry-pick occasional fixed, e.g. whenever they find yet another bug related to restrict pointers in LLVM (which happens regularly 😅), but otherwise there should be some base LLVM roll... I'm not sure how to find that base either, maybe Thomas can help?

@RReverser
Copy link
Collaborator

This commit in April seems telling at least - might try to use it as a sync point: rust-lang/llvm-project@757752f

@tlively
Copy link
Member

tlively commented Jun 9, 2021

Aha, I was confused about the tags. I thought that llvmorg-12-init would be the base commit for the line of LLVM 12 release candidates, but it's actually the commit where they first bumped the trunk version number to 12 and started making changes that wouldn't appear in LLVM 11. So the tag we need to look for to find the closest mainline match to LLVM 12 is actually llvmorg-13-init.

@tlively
Copy link
Member

tlively commented Jun 9, 2021

$ python3 ./emscripten-releases/release-util.py llvm-project llvmorg-13-init
2.0.13 (emscripten-releases rev ce0e4a4d1cab395ee5082a60ebb4f3891a94b256) is the first tag that contains llvm-project rev 305648b8d0d70f4bb1c38165b893cd303a74979a

@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants