Skip to content

build/pkgs/ccache: Update to 4.10.2 #39119

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

Merged
merged 2 commits into from
Dec 22, 2024
Merged

build/pkgs/ccache: Update to 4.10.2 #39119

merged 2 commits into from
Dec 22, 2024

Conversation

gmou3
Copy link
Contributor

@gmou3 gmou3 commented Dec 11, 2024

I also modified sdh_cmake slightly, because inputting . and .. together issued a warning.

Resolves #35872.

@gmou3 gmou3 changed the title Update ccache to 4.10.2 build/pkgs/ccache: Update to 4.10.2 Dec 11, 2024
Copy link

github-actions bot commented Dec 11, 2024

Documentation preview for this PR (built with commit 02273ef; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 15, 2024

I tested the branch (and the new ccache) on a mac. Did you test it on any linux?

@gmou3
Copy link
Contributor Author

gmou3 commented Dec 15, 2024

Yes, seems to work fine.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

OK. LGTM.

Thanks.

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 15, 2024
sagemathgh-39119: `build/pkgs/ccache`: Update to 4.10.2
    
I also modified `sdh_cmake` slightly, because inputting `.` and `..`
together issued a warning.

Resolves sagemath#35872.
    
URL: sagemath#39119
Reported by: gmou3
Reviewer(s): gmou3, Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 19, 2024
sagemathgh-39119: `build/pkgs/ccache`: Update to 4.10.2
    
I also modified `sdh_cmake` slightly, because inputting `.` and `..`
together issued a warning.

Resolves sagemath#35872.
    
URL: sagemath#39119
Reported by: gmou3
Reviewer(s): gmou3, Kwankyu Lee
@vbraun vbraun merged commit 47df08e into sagemath:develop Dec 22, 2024
15 of 44 checks passed
@gmou3 gmou3 deleted the ccache branch December 25, 2024 16:35
@enriqueartal
Copy link
Contributor

I had an issue in Fedora 41, ccache installation did not work having some issue with Xxhash. It worked after installing system package xxhash-devel; should this dependency appeared somewhere? On the other side system package ccache is installed and a fedora.txt exists in the corresponding distro folder, but it is ignored.

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 1, 2025

See https://groups.google.com/g/sage-devel/c/ZZIvoEHm5hg for the issue.

@dimpase
Copy link
Member

dimpase commented Jan 23, 2025

Why does this PR touch a lot a files not related in any way to ccache?

@dimpase
Copy link
Member

dimpase commented Jan 23, 2025

Another problematic change is that ccache now needs xxhash, and it tries to download it from the internet - something that probably breaks the rules for optional packages. And it also is flaky, see
https://groups.google.com/d/msgid/sage-release/CAD0p0K51VNkbovm1De3arXt18E9rpoP%2B%3DWO1414Cb-b_FP0%3D6Q%40mail.gmail.com

@gmou3
Copy link
Contributor Author

gmou3 commented Jan 23, 2025

Why does this PR touch a lot a files not related in any way to ccache?

I also modified sdh_cmake to not automatically run cmake in the current directory. This allows us to follow a package's installation instructions as recommended by the author (e.g., possibly cmake ..).

Another problematic change is that ccache now needs xxhash, and it tries to download it from the internet - something that probably breaks the rules for optional packages. And it also is flaky, see https://groups.google.com/d/msgid/sage-release/CAD0p0K51VNkbovm1De3arXt18E9rpoP%2B%3DWO1414Cb-b_FP0%3D6Q%40mail.gmail.com

Indeed, this is an issue. See #39226 for the proposed solution.

@dimpase
Copy link
Member

dimpase commented Jan 23, 2025

The main issue with this PR is that now for some people installation of the optional package ccache is triggered without them asking for it! This is a bug.

Please see e.g. https://groups.google.com/g/sage-release/c/muFP_3aonHc/m/014QJ6j9DQAJ
and other messages in https://groups.google.com/g/sage-release/c/muFP_3aonHc/m/i7P0R-o_AQAJ

@gmou3
Copy link
Contributor Author

gmou3 commented Jan 23, 2025

That's unexpected, its optional status hasn't changed.
I'll try a clean build on my machine later to see if I can reproduce the issue (did you try this by any chance?).
I can't post on the discussion you shared, but it seems that one of the two users has a problem relevant to ccache while the other does not.

@dimpase
Copy link
Member

dimpase commented Jan 23, 2025

That's unexpected, its optional status hasn't changed. I'll try a clean build on my machine later to see if I can reproduce the issue (did you try this by any chance?). I can't post on the discussion you shared, but it seems that one of the two users has a problem relevant to ccache while the other does not.

no, at least 2 users, @JohnCremona and @EmmanuelCharpentier report unwanted installation of ccache. It could be they run same or similar versions of Ubuntu.

@EmmanuelCharpentier
Copy link
Contributor

EmmanuelCharpentier commented Jan 23, 2025 via email

@JohnCremona
Copy link
Member

My machine runs Ubuntu 22

@dimpase
Copy link
Member

dimpase commented Jan 23, 2025

Why does this PR touch a lot a files not related in any way to ccache?

I also modified sdh_cmake to not automatically run cmake in the current directory. This allows us to follow a package's installation instructions as recommended by the author (e.g., possibly cmake ..).

I don't think it's needed any more. Early versions of cmake required a separate build directory, and these instructions might be a leftover from these early days.

Did you try without making these changes?


I don't see this problem (of unrequested building of ccache spkg) on Gentoo, so this makes for an "interesting" platform-specific bug...

@gmou3
Copy link
Contributor Author

gmou3 commented Jan 24, 2025

Why does this PR touch a lot a files not related in any way to ccache?

I also modified sdh_cmake to not automatically run cmake in the current directory. This allows us to follow a package's installation instructions as recommended by the author (e.g., possibly cmake ..).

I don't think it's needed any more. Early versions of cmake required a separate build directory, and these instructions might be a leftover from these early days.

Did you try without making these changes?

cmake requires a build directory, and often .. is used. Not only ..

Not making such changes would require me to diverge from the recommended ccache instructions, but I wanted to avoid the uncertainties of this. I think this makes the translation of build instructions easier and clearer for all packages. cmake becomes sdh_cmake and that's it.

I don't see this problem (of unrequested building of ccache spkg) on Gentoo, so this makes for an "interesting" platform-specific bug...

So it probably isn't a result of this PR. Maybe the failure just made the building of ccache apparent.

@dimpase
Copy link
Member

dimpase commented Jan 24, 2025

So it probably isn't a result of this PR. Maybe the failure just made the building of ccache apparent.

@JohnCremona - could you please check whether reverting this PR (i.e. the commits at https://github.com/sagemath/sage/pull/39119/commits) fixed the issue of the unasked for build of ccache ?

@JohnCremona
Copy link
Member

JohnCremona commented Jan 27, 2025

So it probably isn't a result of this PR. Maybe the failure just made the building of ccache apparent.

@JohnCremona - could you please check whether reverting this PR (i.e. the commits at https://github.com/sagemath/sage/pull/39119/commits) fixed the issue of the unasked for build of ccache ?

Sorry for the delay, I was away. I will try.

By reverting the PR, do you mean checking out the commit before the first of these and building from scratch?

I did solve the problem by unsetting that macro, so is it necessary to do more?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants