In D145258#4172488, @GMNGeoffrey wrote:

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.

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.

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

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

In D143320#4108552, @aaronmondal wrote:

Regarding the WORKSPACE logic, that sounds good. I'll try to get something like that working but that may take me a while. Tbh I'm not really using WORKSPACES myself and instead using the bzlmod patch series that I tried upstreaming a while ago but I think they got kinda dropped and would probably need some rework at this point. (A working bazel module for a registry is here with sample usage here).

In D143351#4108160, @aaronmondal wrote:

This shouldn't affect anyone using default Bazel configs. The functionality that depends on libxml is disabled (as in, disabled on the preprocessor level). Not having xml2 support disables symbols in llvm/lib/WindowsManifest/WindowsManifestMerger.cpp, but linking xml2 it here without setting LLVM_ENABLE_XML2 has no effect.

This functionality should currently only be usable if you manually patched the Bazel configs.

Ultimately I'd like to make this a configurable flag depending on/setting LLVM_ENABLE_XML2, but I think the CMake-to-Bazel scripts are't ready to support this yet. It's probably better to wait for https://reviews.llvm.org/D143295 to get through first and then add configurability separately.

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.

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:

In D142158#4067059, @rupprecht wrote:

I think I see why we don't see this failure internally (maybe), but I'm clueless as to why the buildkite builder or local builds don't complain. Anyway, thanks!

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

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?

LGTM (+/- nits), but maybe good to get review from someone more closely associated with the TestingSupport library

It seems like the same logic would extend to the CMake build. Could we make the same change there?

LGTM with some nits

I just remembered about this. I think you can land it. -Werror is turned on explicitly on the CI. @aeubanks to confirm

In D119547#3908195, @gchatelet wrote:

edit Even if Bazel is able to build make, m4 is required to build gmp. It's actually a known issue : https://github.com/bazelbuild/rules_foreign_cc/issues/755
So for now we need at least m4, autoconf and automake to be installed on the machine anyways. So let's assume that make is available as well.

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?

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.

@aeubanks FYI

Nice cleanup, thanks :-)

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.

In D129899#3664764, @MaskRay wrote:

I want to mention the Bazel build is an unofficial build system and keeping it work isn't the community's responsibility.
Keeping it work is appreciated by folks who use the Bazel build, though.

In D129899#3664760, @Amir wrote:

I see. I know that bazel build is enabled in pre-merge checks but they are flaky at times, and may not trigger for direct pushes. I may look into adding a buildbot configuration with bazel build. If you have any examples of using bazel in buildbot, please let me know.

In D129899#3664216, @Amir wrote:

Couple of questions:

1. Do I understand it correctly that BUILD.bazel file need to be kept in sync with CMakeLists.txt?

In D127459#3571908, @Mogball wrote:

Yes, I'll be taking at look at the CMake names afterwards. This one goes after the low-hanging fruit, mostly, in the bazel targets. I'll probably do a comprehensive review to make sure everything lines up in bazel and CMake

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.

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.

LGTM, thanks :-)

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

LGTM

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.

In D125096#3508104, @vibhutisawant wrote:

Hi @GMNGeoffrey sorry for the trouble, I uploaded the patch using GUI, and possibly something might have been missed. Will take care next time. Thanks :)

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

Let me know if you need me to land this for you (see https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

In D123481#3442718, @kuhar wrote:

+1, I had to patch this locally in my project for exactly the reasons you mentioned

In D119547#3369973, @gchatelet wrote:

@GMNGeoffrey I'm having an issue that you may know how to solve.
.bazelrc automatically appends -Wall -Werror to everything that is built.

• Math tests depend on mpfr,
• mpfr (and gmp) are built from source using rules_foreign_cc,
• For hermeticity rules_foreign_cc compiles make

I could tweak the ./configure options for mpfr and gmp and explicitly disable -Werror (see file "mpfr.BUILD" and "gmp.BUILD") but I cannot tweak the compile options for when rules_foreign_cc compiles make.
I could work around this by preventing rules_foreign_cc to compiles make in the first place, but then the build is not hermetic anymore...

register_built_tools = False,
register_default_tools = False,

What do you think? Is .bazelrc too strict? Is there a clean way to work around this without lowering the error level?

LGTM, thanks

@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

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).

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 :-)

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 <46229924+jonmeow@users.noreply.github.com>"?

Thanks for the cleanup :-)

LGTM, thanks

This LGTM, but I would wait for Peter to weigh in to make sure.

In D115533#3221708, @jhenderson wrote:

@GMNGeoffrey, (or anybody else) did you land this change yet? If not, I can probably arrange for it this week.

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.

LGTM

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

Use type=os.path.abspath