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

Please push tags to the LLVM fork repo #45021

Closed
infinity0 opened this issue Oct 4, 2017 · 8 comments
Closed

Please push tags to the LLVM fork repo #45021

infinity0 opened this issue Oct 4, 2017 · 8 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR.

Comments

@infinity0
Copy link
Contributor

infinity0 commented Oct 4, 2017

I am sorry for filing this here but the "issues" page on https://github.com/rust-lang/llvm/ is disabled.

From a packaging perspective it would be good to have the LLVM release tags (major and minor releases) also mirrored to https://github.com/rust-lang/llvm/ - then it would be much more obvious which commits Rust needs compared to a clean upstream LLVM tarball.

At the moment rustc 1.20.0 has 2 failing tests on amd64 on Debian and I haven't yet had time to figure out which LLVM revisions are needed, or if it's because of some other issue. Every time I have to crawl through the git logs and manually compare stuff with upstream and Debian versions of LLVM.

@cuviper
Copy link
Member

cuviper commented Oct 4, 2017

I just add upstream locally, e.g. git remote add llvm https://git.llvm.org/git/llvm. Or vice versa, add a rust-llvm remote to my existing upstream llvm repo.

Note however that the official git mirror doesn't have release tags at all! You do get stable branches though, like release_39 and release_40, which are generally good enough to compare with.

@infinity0
Copy link
Contributor Author

Hm, I am not sure that helps that much, it looks like the commits are quite a bit different. Checking out the LLVM for Rust 1.20.0 (commit a5ef0696) then:

$ git rev-list HEAD ^upstream/release_40 | wc -l
2653

Anything else you're doing to diff this quickly? I have been going over the Debian LLVM source code manually then seeing which commits were applied.

@infinity0
Copy link
Contributor Author

Oh right, it looks like most of those are from 5401fdf23ed "Merge fastcomp @ 1.37.10". The other parent is upstream/release_40.

@infinity0
Copy link
Contributor Author

infinity0 commented Oct 4, 2017

#36356 seems abandoned for the time being. :(

In that case, it would be good in the future to tag commits like rust-lang/llvm@5401fdf as upstream-40+fastcomp-1.37.10 which would appear very obviously in a git log avoiding people having to scroll through 2500 commits.

(It would be especially awkward if upstream/release_40 had moved on by more commits in the meantime. Luckily that wasn't the case here.)

@est31
Copy link
Member

est31 commented Oct 5, 2017

@infinity0 @cuviper the LLVM used is a custom fork of upstream LLVM with fastcomp added on top, and with a few backported patches from newer LLVM versions that are needed for various reasons. The long term plan is to get rid of fastcomp by leveraging the still evolving wasm backend of LLVM. See #44006 if you want to track progress. Also, AFAIK there is nothing preventing Rust from working with an LLVM without fastcomp support, not sure though about the details.

@cuviper
Copy link
Member

cuviper commented Oct 5, 2017

and with a few backported patches from newer LLVM versions that are needed for various reasons.

For me, these "various reasons" are why I bother comparing at all. Yes, Rust is fine with a more vanilla LLVM, and we use that for distro packaging, but I always want to evaluate whether any of those patches are things that we might want to backport to the distro LLVM library too.

@infinity0
Copy link
Contributor Author

infinity0 commented Oct 5, 2017

I totally understand why Rust keeps its own fork. The reason why I have to do the comparisons is because Rust's test suite often fails (or worse the whole build fails) with an unforked LLVM, which is what Debian has. Then I have to ask the Debian LLVM maintainer to cherry-pick various commits that Rust has already applied. It's easier to do this if the git log has the fork-points clearly labelled.

@arielb1 arielb1 added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Oct 5, 2017
@Mark-Simulacrum
Copy link
Member

I'm going to go ahead and close this as I think we've been relatively good about tagging for the last few upgrades, but if this comes up again I'd ask in #infra on Discord and someone can probably go and tag the relevant commits for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR.
Projects
None yet
Development

No branches or pull requests

5 participants