[CI] include utils in monolithic scripts

[makslevental]

This PR adds *_UTILS to the monolithic scripts so that follow-up PRs can use utils in install flow tests.

[nikic]

LLVM_BUILD_UTILS and LLVM_INCLUDE_UTILS are already enabled by default. And we're not installing anything, so LLVM_INSTALL_UTILS shouldn't really do anything either. Overall I'm confused what difference this change actually makes.

[makslevental]

#157944 adds an install test. Also yes the first two are on by default but I thought it better to be explicit.

[boomanaiden154]

This PR adds *_UTILS to the monolithic scripts so that follow-up PRs can use utils in install flow tests.

More information on what exactly this is intended to test would be good.

It also looks like this is on by default?

llvm-project/llvm/CMakeLists.txt

Line 849 in 89e32ac

option(LLVM_BUILD_UTILS

[makslevental]

More information on what exactly this is intended to test would be good.

It also looks like this is on by default?

#157944 adds an install test. Also yes the first two are on by default but I thought it better to be explicit.

[boomanaiden154]

#157944 adds an install test. Also yes the first two are on by default but I thought it better to be explicit.

What happens to that test if LLVM_INSTALL_UTILS is set to false?

[makslevental]

What happens to that test if LLVM_INSTALL_UTILS is set to false?

The test already currently depends on targets for FileCheck, count, and not

https://github.com/llvm/llvm-project/blob/main/mlir/examples/standalone/test/CMakeLists.txt#L13

So it fails to configure successfully when run against a distro that doesn't have those targets (ie what can currently be installed by the monolithic scripts).

[boomanaiden154]

So it fails to configure successfully when run against a distro that doesn't have those targets (ie what can currently be installed by the monolithic scripts).

So the test will fail if LLVM_INSTALL_UTILS isn't set to true?

[makslevental]

So the test will fail if LLVM_INSTALL_UTILS isn't set to true?

Yes that's correct.

[boomanaiden154]

So this will break everyone who sets LLVM_ENABLE_EXAMPLES=ON but not LLVM_INSTALL_UTILS=ON? That kind of seems like a problem.

[makslevental]

So this will break everyone who sets LLVM_ENABLE_EXAMPLES=ON but not LLVM_INSTALL_UTILS=ON? That kind of seems like a problem.

What is this? This PR doesn't break anything/anyone because it's NFC because as nikita mentioned, currently monolithic doesn't actually install anything.

That test in the follow-up PR won't break anyone because it will only run in CI here (it's discussed in the PR description).

[boomanaiden154]

What is this? This PR doesn't break anything/anyone because it's NFC because as nikita mentioned, currently monolithic doesn't actually install anything.

This as in the test that gets added. I'm pretty strongly opposed to a test that only runs in premerge. The test needs to be easily reproducible locally without a bunch of subtle undocumented CMake requirements.

[makslevental]

This as in the test that gets added

There is no test added in this PR - this PR is completely NFC (it's been explained twice). The other PR is in draft and not finished. You're free to comment on that PR when it's ready for review but these two PRs are fundamentally orthogonal.

[boomanaiden154]

There is no test added in this PR - this PR is completely NFC (it's been explained twice). The other PR is in draft and not finished. You're free to comment on that PR when it's ready for review but these two PRs are fundamentally orthogonal.

How are these orthogonal? The only reason to land this PR is for #157944.

[makslevental]

How are these orthogonal? The only reason to land this PR is for #157944.

Because the other PR isn't finished and I'm going to refactor it to use an explicit CMake configure arg instead of an env variable but I will still need this change.

[boomanaiden154]

I'll look at this again when the other patch is ready.

[makslevental]

FYI I don't see how you can keep blocking this change - the facts are

1. This PR is completely NFC at HEAD;
2. You have no technical reason for blocking the change other than "I don't like it";
3. The other PR is up and you simply air these concerns there instead of here because your concerns are about that one.

Note, you're free to not approve it - that's fine. But you have no basis for blocking.

[makslevental]

I'll look at this again when the other patch is ready.

I'm sorry is it somewhere in the charter/rules/guidelines that that's how we're doing PRs now? Typically, quite the opposite occurs - orthogonal changes are requested to be factored out and landed separately when they're independent.

[nikic]

The thing is that this PR just doesn't make any sense in isolation. It it needed if and only if the other PR lands. If the other PR does not get approved, then this PR just leaves us with a bunch of unnecessary and confusing options in the CI config.

I don't think there is much point in splitting out this change, this can just be folded into the other PR and reviewed as part of it.

[boomanaiden154]

You have no technical reason for blocking the change other than "I don't like it";

I don't have the relevant context to know if this patch makes sense because the only thing that depends on this functionality is not yet ready for review.

The other PR is up and you simply air these concerns there instead of here because your concerns are about that one.

It's marked as a draft and you explicitly told another reviewer Also, in general, I would appreciate it if you didn't review my PRs until they are marked Ready for review..

I'm sorry is it somewhere in the charter/rules/guidelines that that's how we're doing PRs now? Typically, quite the opposite occurs - orthogonal changes are requested to be factored out and landed separately when they're independent.

Patches don't exist in isolation. There is context surrounding them. I think it is reasonable to request that the context is available and ready to be looked at before reviewing a change. Sure, "orthogonal" changes should be factored out when separable, but these changes still don't exist in a vacuum.

[makslevental]

closing because folded into #157944