- User Since
- Jan 15 2020, 6:41 PM (166 w, 1 d)
Wed, Mar 22
Mon, Mar 6
Hmmm actually our internal version of these build files also uses alwayslink with the comment "Registration of the clang-tidy checker modules relies on the linker, so we need to enforce linking". That comment was written in 2014 though, so things may have changed. Let me try seeing what tests fail if I remove that internally.
Feb 16 2023
Feb 10 2023
Looks like there's still an issue with mpfr. One advantage to adding at the package level is that it will also apply for downstream projects using LLVM. So if someone is building via their own project and editing an LLVM submodule or something then they would still have layering enforced. Just a thought
Feb 9 2023
layering_check has already been turned on at the package level for the packages that are layering clean (e.g. https://github.com/llvm/llvm-project/blob/main/utils/bazel/llvm-project-overlay/llvm/BUILD.bazel#L15). You could add more
Feb 6 2023
Aside: this patch appears to be missing a bunch of context in the diff. Could you try reuploading or something?
Hmm I'm trying to remember this history on this one too. I think this was added after hitting a real bug on Windows... Again @chandlerc
Ok it looks like the conversation about licensing implications was in some ephemeral format or something, as I can't find it. And zlib actually has a permissive license. Maybe Chandler will comment, but my preference would be that we keep the ability to depend on the system zlib version
I like that this fixes this to use a proper flag, but support for using the system version was deliberate and static-linking an external archive had interesting licensing implications. Let me dig up the history. Hermeticity is indeed an important consideration, but building with system dependencies is also key functionality. @chandlerc, who added this originally.
Jan 23 2023
Well I just spent quite a while fiddling with the zlib setup and didn't come up with something better than disabling layering check on Support. The issue is that we want the zlib rule to add in LLVM's define as well (at least as currently configured). Otherwise we could do an alias or cc_import rule. The latter might even be nicer for system zlib than the current hardcoding of linkopts, which I think is unfriendly to Windows. Trying to create a header to re-export the zlib header doesn't work because it needs to be called "zlib.h" and then it just recursively includes itself. I'm annoyed (but not that shocked) that Bazel doesn't have a way to declare a header re-exported from the build rule. I think a better solution than disabling layering would involve reworking the way we do configurable dependencies here in general.
Hmm breaking zlib_external isn't great. I actually ran into another issue when I tried to repro this:
Jan 19 2023
I'm seeing a bunch of layering failures when trying to integrate this into IREE. We're building with clang-16 which seems to have gotten much better at finding these issues (IIRC there used to be a bug that prevented this). I'm surprised Google hasn't hit this issue with the internal build as well, since I thought we were using a very new clang. Maybe we could use a newer clang in the Bazel build bot? Disadvantage is that it might stop working with an older Clang
Jan 17 2023
This looks reasonable, but I'm not familiar with @rules_pkg. Could you add more explanation to the patch description about the goals of this change?
Jan 11 2023
LGTM (+/- nits), but maybe good to get review from someone more closely associated with the TestingSupport library
Jan 9 2023
It seems like the same logic would extend to the CMake build. Could we make the same change there?
Jan 3 2023
Nov 22 2022
Nov 16 2022
LGTM with some nits
Nov 9 2022
Nov 8 2022
I just remembered about this. I think you can land it. -Werror is turned on explicitly on the CI. @aeubanks to confirm
Nov 3 2022
Are there practical portability concerns about doing this in Python? I'm not aware of a platform where Bazel runs that Python doesn't. I do like pulling this directly from the CMake in the cases where the values are set there. I think there was discussion at some point of whether it would be easier if these values were encoded somewhere in a simple text file. Here we go: https://discourse.llvm.org/t/rfc-centralized-location-for-version-information/65295. Might be good to follow-up on where that went
It seems like you're synced to a bad commit that's broken for other reasons. Could you rebase and try the pre-merge checks again?
I think this is coming from my complete ignorance of bzlmod, but why exactly do we have a separate llvm-project-overlay repository here? Why isn't it pulling from utils/bazel?
Oct 28 2022
I'm not familiar with how bzlmod or the central registry are going to work, but just looking at the content of this patch, I think there is a problem. I think this adds a dependency on exactly the name "llvm-raw" being used when this is imported by external projects. As currently written, it's a relative reference, which has no dependence on this name. I took some care when setting all of this up to not make user-defined strings like that load-bearing, so I think this is undesirable. llvm-raw is kind of an implementation detail name that no one should care about too much though and is unlikely to be referenced by any other dependency, so it isn't too bad to bake it in. It was more important for the name of the final llvm-project Bazel repository where there were already multiple names being used in the wild. The fact that dependency names are user-defined and not necessarily consistent between projects has always been a failing in the Bazel dependency management system though, and it looks like perhaps bzlmod and this registry are attempting to establish canonical names for projects, more aligned with how package managers like pip work. If that's the world we're headed for, then it seems like baking this in may be the right thing to do, but I'm unsure.
Oct 10 2022
Sep 22 2022
Nice cleanup, thanks :-)
Aug 29 2022
So the issue we're trying to solve is that the pre-merge check is still running on the old runners and using "--config=rbe", right? Please add that info to the description and link your PR (https://github.com/google/llvm-premerge-checks/pull/414) attempting to fix this in the pre-merge checks repo.
Jul 20 2022
Jul 19 2022
Jun 9 2022
Thank you! Can you confirm that the names we're switching to are actually the consistent pattern in CMake as well? One issue I encountered before when cleaning this up was that the CMake names weren't consistent with each other across the project, so it was a little unclear what should be changed.
Jun 1 2022
I see you've already landed and reverted this. Apologies I didn't get to review in time. In previous iterations, we did have a python script for writing these config files that came from TensorFlow. I think it was less good than this one in terms of error detection. @chandlerc had strong opinions about not using a custom script here, I believe arguing that behavior that is "like CMake but not quite CMake" was likely to be error-prone. FWIW, I think this is overall an improvement (especially with its error detection instead of our existing config file copies), but I think it's worth bringing this up again. I think I even looked at using the GN script in Bazel and Chandler was against it.
May 26 2022
LGTM, thanks :-)
May 20 2022
To add it, we'd need to upgrade to 5.x, which is nontrivial because of the current use of remote execution, unfortunately. We're looking into moving these builders over to building locally, but when I tried it out I ran into some issues: https://github.com/google/llvm-premerge-checks/pull/394. @goncharov is working on making this all better supported, I believe
The CI is still using Bazel 4.0, so it doesn't have linux_riscv64
May 16 2022
May 13 2022
https://github.com/google/llvm-premerge-checks/pull/400 turns this on for the pre-merge checks and I've updated the continuous build. I want to make sure the pre-merge checks are doing what we expect. Mehdi, perhaps you could push an update to this change? That should re-run the pre-merge checks and we can confirm they're using -Werror. Alternatively, any patch that touches the bazel files or is by someone in the Bazel project would do it. I just didn't want to send draft patches because people have griped about email traffic in the past.
May 12 2022
May 11 2022
However you created this patch, it's causing me issues when trying to arc patch it. It looks like it's missing a lot of a metadata (in particular, a base revision). I've manually recreated the revision and reuploaded it here, but FYI for future reference
Recreating the patch with appropriate metadata
May 10 2022
Let me know if you need me to land this for you (see https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)
Mar 28 2022
Mar 9 2022
Feb 17 2022
Feb 11 2022
@chandlerc any opinions on having multiple methods for depending on external libraries? Version 0.7 doesn't inspire confidence
I think we should provide at least the option for users to use these from their system, as we do with zlib
Feb 4 2022
Feb 3 2022
Seems like the relevant patch was reverted? https://github.com/llvm/llvm-project/commit/dbf47d227d080e4eb7239b589660f51d7b08afa9 -> https://buildkite.com/llvm-project/upstream-bazel-rbe/builds/19349. Probably good to keep this around anyway, since I suspect it will be re-landed at some point. This looks like a reasonable fix for that, so I'll go ahead and approve, but if you need to land again, please rebase and wait for pre-merge checks to pass (for some reason they're taking a while on this commit).
Feb 2 2022
Looks like this was fixed in https://github.com/llvm/llvm-project/commit/4a6c9b5686. You also generally don't want to list headers in multiple places. Rather, have those things depend on targets providing that header (as was done in the linked commit).
If you'd like to rebase this and see what's still outstanding, I can take another look :-)
Feb 1 2022
Jan 31 2022
Jan 26 2022
LGTM. Thanks for the fix. Can you add the explanation to the description? Also, do you need me to land this for you? If so, do you want it committed with the author information from this commit "jonmeow <firstname.lastname@example.org>"?
Thanks for the cleanup :-)
Jan 25 2022
Jan 12 2022
This LGTM, but I would wait for Peter to weigh in to make sure.
Jan 5 2022
IMO it would be better to have a way for the person specifying the conversion target to say "get rid of these ops if you can". Unrealized conversion cast is a bit of an odd duck, but it seems that "get rid of this during dialect conversion if possible" is more often a property of the conversion than a property of the op. I guess it could be either or both (where unrealized conversion is the relatively special case where it makes sense to have it on the op). It also feels like an arbitrary limitation that such an op can only be removed with folding and not with canonicalization or some other pattern.
Dec 23 2021
Dec 20 2021
The only pre-merge failure is LLVM :: Bindings/Go/go.test which is failing for lots of people, as discussed in https://groups.google.com/g/llvm-dev/c/o07dxO7Y-CA. Going to land past that
Other than that, LGTM
Rebase on head
Dec 15 2021