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

Deprecate the master branch #1739

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Deprecate the master branch #1739

merged 2 commits into from
Dec 9, 2024

Conversation

MarcelKoch
Copy link
Member

This PR adds warnings when the master branch is used. The master branch will not be updated after 2025. It will produce both CMake warnings and compiler messages (the #warning directive is only available in c++23).

The main reason for deprecating the master branch is that the release process is more complicated with the master branch. The develop and master branch have no common history, so merging develop into master will reapply every commit in develop since the 1.0 release.

@MarcelKoch MarcelKoch self-assigned this Dec 4, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. mod:core This is related to the core module. labels Dec 4, 2024
Comment on lines 20 to 21
#pragma message "The version tag " GKO_VERSION_TAG " is deprecated and will stop receiving updates after 2025. " \
"Please use the main branch for the latest release, or the develop branch for the latest development updates."
Copy link
Member

Choose a reason for hiding this comment

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

In MSVC, you need to use #pragma message WARN( ...) from lower_trs.hpp

Copy link
Member Author

Choose a reason for hiding this comment

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

Is WARN a macro from us, because it doesn't seem to be part of msvc: https://learn.microsoft.com/en-us/cpp/preprocessor/message?view=msvc-160?

Copy link
Member

Choose a reason for hiding this comment

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

I am also checking, but do not get anything specifying WARN when we have that in the beginning.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe set the tag as master temporary to check whether it report the message on MSVC

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good suggestion, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

You can see here that it does complain quite a lot: https://gitlab.com/ginkgo-project/ginkgo-public-ci/-/jobs/8557710509

@MarcelKoch MarcelKoch added this to the Ginkgo 1.9.0 milestone Dec 4, 2024
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

I'm a bit unsure about the wording - what is named version tag here is only used to refer to branches in the error messages. I would talk about branches instead.

@MarcelKoch
Copy link
Member Author

Yes, but the cmake variable is called Ginkgo_VERSION_TAG, so stuck with that. I don't know if this could be something other than a branch, so I didn't want to assume anything.

However, if we really use the variable only for branch names, then I would be fine to change the message.

@MarcelKoch MarcelKoch added the 1:ST:ready-for-review This PR is ready for review label Dec 5, 2024
- if: $RUN_CI_TAG && ($STATUS_CONTEXT == "full" || $CI_COMMIT_BRANCH == "master" || $CI_COMMIT_BRANCH == "develop" || $CI_COMMIT_TAG)
- if: $RUN_CI_TAG && ($STATUS_CONTEXT == "full" || $CI_COMMIT_BRANCH == "main" || $CI_COMMIT_BRANCH == "develop" || $CI_COMMIT_TAG)
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional behavior? only run the quick test for master branch

Copy link
Member Author

Choose a reason for hiding this comment

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

There should be no CI on the master anymore. We don't need to test the same commit with three pipelines.

Copy link
Member

Choose a reason for hiding this comment

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

If so, maybe also add one to quick_test_condition

- if: $RUN_CI_TAG && $CI_COMMIT_BRANCH == "master"
  when: never

CMakeLists.txt Outdated Show resolved Hide resolved
@MarcelKoch MarcelKoch merged commit 3fa5fd1 into develop Dec 9, 2024
4 of 10 checks passed
@MarcelKoch MarcelKoch deleted the deprecate-master branch December 9, 2024 13:38
@ginkgo-bot
Copy link
Member

Error: PR already merged!

1 similar comment
@ginkgo-bot
Copy link
Member

Error: PR already merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-for-review This PR is ready for review mod:core This is related to the core module. reg:build This is related to the build system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants