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

Fix zlib-ng build with cmake 3.31.0 #110359

Merged
merged 15 commits into from
Dec 5, 2024
Merged

Fix zlib-ng build with cmake 3.31.0 #110359

merged 15 commits into from
Dec 5, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented Dec 3, 2024

We are seeing warnings like these (with cmake 3.31 in github actions):

CMake Deprecation Warning at D:/a/runtime/runtime/runtime/src/native/external/zlib-ng/CMakeLists.txt:1 (cmake_minimum_required):
  Compatibility with CMake < 3.10 will be removed from a future version of
  CMake.

When we started src/native/external projects, the idea was that we will keep our configs outside the vendor directory and we won't be using their cmake configs if they are providing one. That made version updates simple and "our" configs were just a flat list of files we want to build in the product. Now we are adding new dependencies with "fetch plan", that uses the cmake config from the vendor dir and we have to keep up with the config issues..

fyi @jkoritzinsky one downside of fetch plan.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 3, 2024
@am11 am11 requested a review from jakobbotsch December 3, 2024 15:33
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

@am11
Copy link
Member Author

am11 commented Dec 3, 2024

@jakobbotsch I'll revert the dummy change in JIT once windows format leg is green.

@am11
Copy link
Member Author

am11 commented Dec 3, 2024

@jakobbotsch, a side note: in https://github.com/dotnet/runtime/actions/runs/12145206891/job/33866308923?pr=110359 we can see repeated downloads:

Downloading 'https://clrjit2.blob.core.windows.net/clang-tools/17.0.6/windows-x64/clang-format.exe' to 'D:\a\runtime\runtime\runtime\artifacts\tools\clang-format.exe'
Downloading 'https://clrjit2.blob.core.windows.net/clang-tools/17.0.6/windows-x64/clang-tidy.exe' to 'D:\a\runtime\runtime\runtime\artifacts\tools\clang-tidy.exe'

first in Build jitutils step and then in Run jitformat.py. Perhaps we can skip the one in build step? Also, clang-tools can be bumped from v17 to v19.

@jakobbotsch
Copy link
Member

I did not see the jit-format Windows run succeeding with this PR, it just seemed to be permanently stuck when I looked. Did you see it succeed?

@jakobbotsch
Copy link
Member

@jakobbotsch, a side note: in https://github.com/dotnet/runtime/actions/runs/12145206891/job/33866308923?pr=110359 we can see repeated downloads:

Downloading 'https://clrjit2.blob.core.windows.net/clang-tools/17.0.6/windows-x64/clang-format.exe' to 'D:\a\runtime\runtime\runtime\artifacts\tools\clang-format.exe'
Downloading 'https://clrjit2.blob.core.windows.net/clang-tools/17.0.6/windows-x64/clang-tidy.exe' to 'D:\a\runtime\runtime\runtime\artifacts\tools\clang-tidy.exe'

first in Build jitutils step and then in Run jitformat.py. Perhaps we can skip the one in build step?

Probably, I'm not really familiar with the setup of this, cc @dotnet/jit-contrib

Also, clang-tools can be bumped from v17 to v19.

As long as it doesn't come with a large amount of churn to the JIT code base...

@am11
Copy link
Member Author

am11 commented Dec 3, 2024

I did not see the jit-format Windows run succeeding with this PR, it just seemed to be permanently stuck when I looked. Did you see it succeed?

The comment above it has a link to green run?

@MichalPetryka
Copy link
Contributor

I did not see the jit-format Windows run succeeding with this PR, it just seemed to be permanently stuck when I looked. Did you see it succeed?

Windows jit-format action has been broken on main for like 2 weeks, it's failing on every single JIT PR.

@jakobbotsch
Copy link
Member

The comment above it has a link to green run?

Ah, ok, looks good then. For some reason when I look at the "Checks" page it shows me a still running jit-format job.

Windows jit-format action has been broken on main for like 2 weeks, it's failing on every single JIT PR.

Yes, @am11 is aiming to fix the failure with this PR.

@am11
Copy link
Member Author

am11 commented Dec 3, 2024

Ah, ok, looks good then. For some reason when I look at the "Checks" page it shows me a still running jit-format job.

Yup, I had to update a file under coreclr/nativeaot and one eng/native to cleanup few other warnings. The problem is that the python script fails when there is even a single config warning. Then GitHub running gets stuck for five hours in 'spawned process exiting with non-zero line' part of the pythong script. Maybe we should try to make the script robust against warnings.

@BruceForstall
Copy link
Member

first in Build jitutils step and then in Run jitformat.py. Perhaps we can skip the one in build step? Also, clang-tools can be bumped from v17 to v19.

It's fine to only download the tools once. It's not clear which download is actually getting used. I'm not familiar with the current job setup; @jkoritzinsky created that.

As for the version change, we shouldn't update unless we are required to. Or it provides large benefits of some kind. At one point there was an idea to share the tools between JIT and non-JIT runtime users, but I don't think that happened (it would make it much harder to update if it did).

@jkoritzinsky
Copy link
Member

I specifically wanted to build jit-utils separately to avoid any problems with cloning and building jitutils within a runtime repo clone (which is what the bootstrap does). Building it within a runtime repo clone can cause issues with global.json and other configuration settings within the repo infra. Isolating it to its own sibling folder ensures that we don't have these issues.

We shouldn't be downloading jitutils again in the Run jitformat.py step. If we are, that's a bug.

@am11
Copy link
Member Author

am11 commented Dec 3, 2024

Now it's downloading once.

I specifically wanted to build jit-utils separately to avoid any problems with cloning and building jitutils within a runtime repo clone (which is what the bootstrap does). Building it within a runtime repo clone can cause issues with global.json and other configuration settings within the repo infra. Isolating it to its own sibling folder ensures that we don't have these issues.

The python script clones it under $TEMP and does the "right thing". It won't clutter the diffs.

@jkoritzinsky
Copy link
Member

You can remove the Checkout jitutils step as well then.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

This looks good to me but I'm not very familiar with any of this, so I will defer to @jkoritzinsky for another approval.

@jakobbotsch jakobbotsch closed this Dec 4, 2024
@jakobbotsch jakobbotsch reopened this Dec 4, 2024
@jakobbotsch jakobbotsch merged commit e99557b into dotnet:main Dec 5, 2024
144 of 151 checks passed
@jakobbotsch
Copy link
Member

Thanks!

@am11 am11 deleted the patch-24 branch December 5, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Infrastructure-libraries community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants