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

Remove old variable in CMake gko_rename_cache #1471

Merged
merged 2 commits into from
Nov 28, 2023
Merged

Conversation

upsj
Copy link
Member

@upsj upsj commented Nov 24, 2023

I wanted to propose this change to get rid of warning messages on my long-standing repositories, be creative in thinking of potential negative side-effects this might have!

This removes the old variable from cache instead of leaving it around, which leads to the error message only being shown once instead of continuously. Removing variables from cache requires modifying the CMakeCache.txt, which isn't something we need to ask of them.

@upsj upsj added the 1:ST:ready-for-review This PR is ready for review label Nov 24, 2023
@upsj upsj requested review from yhmtsai and a team November 24, 2023 17:42
@upsj upsj self-assigned this Nov 24, 2023
@upsj upsj marked this pull request as ready for review November 24, 2023 17:42
@ginkgo-bot ginkgo-bot added the reg:build This is related to the build system. label Nov 24, 2023
Copy link

codecov bot commented Nov 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0775637) 91.33% compared to head (9aec535) 89.30%.
Report is 15 commits behind head on develop.

❗ Current head 9aec535 differs from pull request most recent head 78454bb. Consider uploading reports for the commit 78454bb to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1471      +/-   ##
===========================================
- Coverage    91.33%   89.30%   -2.04%     
===========================================
  Files          688      688              
  Lines        56098    56336     +238     
===========================================
- Hits         51239    50310     -929     
- Misses        4859     6026    +1167     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmake/rename.cmake Outdated Show resolved Hide resolved
cmake/rename.cmake Outdated Show resolved Hide resolved
@upsj upsj added the 1:ST:no-changelog-entry Skip the wiki check for changelog update label Nov 27, 2023
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

As we discussed in #1350 , I do not have strong opinions on either one.
I do not get why it introduces the issue. It only throws the warning and should not block any operation. user can unset the variable from cmake command by -U<parameter>

@upsj
Copy link
Member Author

upsj commented Nov 27, 2023

Showing the warning once is fine, but since we can easily fix this, showing it repeatedly is noise. We should keep the amount of noise small to highlight more important warning messages. If a user only had the old variable in their CMakeCache.txt, they won't care we changed it. If they always use -DGINKGO_BUILD_DPCPP=ON in their CMake invocation, they will get the warning every time, highlighting the issue for them.

Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

I can not comment on whether user do not care or not on some hidden changes.

@upsj
Copy link
Member Author

upsj commented Nov 27, 2023

As a developer, I definitely care 😄 I think deprecation warnings make sense if somebody is actively doing something deprecated, but if it's just a variable they still have in their CMakeCache.txt, we can fix it for them.

@upsj upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Nov 28, 2023
@upsj upsj merged commit f40f46d into develop Nov 28, 2023
9 of 15 checks passed
@upsj upsj deleted the cmake-deprecate-remove branch November 28, 2023 18:22
@ginkgo-bot
Copy link
Member

Error: PR already merged!

Copy link

sonarcloud bot commented Nov 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:no-changelog-entry Skip the wiki check for changelog update 1:ST:ready-to-merge This PR is ready to merge. reg:build This is related to the build system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants