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

Do not include unreleased changelog in released versions. #10085

Merged
merged 3 commits into from May 13, 2019

Conversation

@Zimmi48
Copy link
Member

commented May 7, 2019

Kind: infrastructure.

This PR introduces a new configure.ml variable specifying if the version that we are building is a released version. If this is the case, then the Changes chapter of the reference manual won't include the “Unreleased changes” initial section. The test-suite will also fail if this variable is set to true but there are remaining unreleased changelog entries, the idea being to remind the RM that all such entries should have been included in the appropriate section of the Changes chapter and removed from doc/changelog before proceeding to tag.

After putting the tag, it is expected that this variable will be turned back to false, although if it is not done immediately, it is not a problem, but as soon as the RM tries to backport a PR with a changelog entry, it will make the test-suite fail.

I documented this in the release-process.md document, and also took the opportunity to advise using git push --tags --dry-run.

  • Corresponding documentation was added / updated.

@Zimmi48 Zimmi48 added this to the 8.10+beta1 milestone May 7, 2019

@Zimmi48 Zimmi48 requested review from vbgl and cpitclaudel May 7, 2019

@Zimmi48 Zimmi48 requested review from ejgallego and coq/doc-maintainers as code owners May 7, 2019

if [ "$(ls $d/*.rst | wc -l)" != "1" ]; then
echo "Fatal: unreleased changelog entries remain in ${d#../../}/"
echo "Include them in doc/sphinx/changes.rst and remove them from doc/changelog/"
exit 1

This comment has been minimized.

Copy link
@SkySkimmer

SkySkimmer May 7, 2019

Contributor

Is this test correct for unreleased versions? ie the test should succeed when is_released=false even if there are extra files.

This comment has been minimized.

Copy link
@Zimmi48

Zimmi48 May 7, 2019

Author Member

Right, I intended to have a mechanism to run this test only when is_a_released_version is true and then I forgot. What is strange to me is that when I ran the test-suite, it passed, even if when I tried to run it manually, it did fail.

This comment has been minimized.

Copy link
@SkySkimmer

SkySkimmer May 7, 2019

Contributor

misc tests are run with current directory test-suite/, not test-suite/misc/

@Zimmi48 Zimmi48 force-pushed the Zimmi48:is-a-release branch from 31c3396 to 192e43f May 7, 2019

@jfehrle
Copy link
Contributor

left a comment

Suppose I add a new tactic and document it. IIUC, this proposal would facilitate hiding the new item in the changelog, but the documentation for the tactic would still be visible anyway. It seems to me you'd want a user-visible tag on the added doc such as (version 8.11). And if we have those in the doc then why not use those in the changelist, too? These tags could be kept around for a while.

Separately, would it make any sense to convert the checklist items in release-process.md into a script? It might reduce operator errors. On the other hand, we only do a handful of releases each year.

@Zimmi48

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Suppose I add a new tactic and document it. IIUC, this proposal would facilitate hiding the new item in the changelog, but the documentation for the tactic would still be visible anyway. It seems to me you'd want a user-visible tag on the added doc such as (version 8.11). And if we have those in the doc then why not use those in the changelist, too? These tags could be kept around for a while.

Nope. I'm not hiding anything. The main idea is to take advantage of the backporting process to never misplace changelog entries as it has happened often in the past. Another important side effect is never having to worry about merge conflicts in CHANGES.md anymore. More about the basic ideas behind this at: https://about.gitlab.com/2018/07/03/solving-gitlabs-changelog-conflict-crisis/

Separately, would it make any sense to convert the checklist items in release-process.md into a script? It might reduce operator errors. On the other hand, we only do a handful of releases each year.

Indeed, that's part of my plan.

@Zimmi48

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

To clarify what I mean by "I'm not hiding anything", this PR is about hiding a supposedly empty unreleased section (so only the title of the section and the division in components). The test being added to the test-suite is about ensuring that one cannot turn the is_a_released_version variable to true if it's not the case that the unreleased changelog is empty.

@Zimmi48 Zimmi48 removed the needs: testing label May 8, 2019

@Zimmi48

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Confirmed (https://gitlab.com/Zimmi48/coq/pipelines/60390418) that with the new version, setting the variable is_a_released_version to true without removing the unreleased changelog entries does make the test-suite fail. https://gitlab.com/Zimmi48/coq/pipelines/60404593 should then confirm that when is_a_released_version is set to true, removing all the unreleased changelog entries does make the test-suite succeed.

Zimmi48 added some commits May 8, 2019

Define a new `is_a_released_version` variable in configure.ml.
Use it to not include unreleased changes when building a released
version.
Add a test that unreleased changelog of released versions is empty.
This test is active only when configure `is_a_released_version` is set
to true. In this case, to pass the test-suite, there must be no
unreleased changelog entries left, i.e. `doc/changelog/*/` must only
contain `00000-title.rst` files.

@Zimmi48 Zimmi48 force-pushed the Zimmi48:is-a-release branch from 192e43f to 60e976a May 8, 2019

@ejgallego

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Could the check just be querying git to see if the current commit has a proper tag as an ancestor?

I'd be cool if we could program the logic and avoid the flag.

@Zimmi48

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I tried to avoid relying on git on purpose in this PR. We can always refine later though. In particular, I'm waiting to see what will be the proposed model for setting the version numbers after the full move to Dune (note that this is one of the main thing that has been delaying any action from me towards more automation: I don't want to have to do it twice).

My main concern with relying on a tag is that if the logic is broken, we only realize it once the tag is put, whereas in this case, the RM would open a PR setting the is_a_released_version to true while bumping the version numbers, and CI would confirm that the change is OK. Of course, if we were relying on standard Dune support, I would be less afraid.

@ejgallego

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I tried to avoid relying on git on purpose in this PR. We can always refine later though. In particular, I'm waiting to see what will be the proposed model for setting the version numbers after the full move to Dune (note that this is one of the main thing that has been delaying any action from me towards more automation: I don't want to have to do it twice).

Makes sense, we could however take the opportunity as to add a comment detailing the TODO for the removal of the manual flag. IMHO the process should become fully automatic in the future, as long as it is manual prone we will always do it wrong.

@Zimmi48

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I don't think that TODO in code are so useful. There is already a few TODO in the release doc, and there is the Release process automation project. I think it is enough for now, isn't it?

@ejgallego

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I think it is enough for now, isn't it?

It not a blocker of course; my concrete fear of introducing this variable is that I'd like to know if it can be derived as a program formula of some kind. Then, I know it can be effectively eliminated later. The current docs don't clarify that.

@Zimmi48

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

This is because I don't know what the process will look like with Dune yet. But even if it is not fully eliminated, as long as it is set and unset automatically, it should be fine, shouldn't it?

@vbgl vbgl modified the milestones: 8.10+beta1, 8.10.0 May 13, 2019

@Zimmi48

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

FTR this PR has been ready for 5 days, and could have been merged anytime since.
@vbgl I'm not really sure what you intend to do for 8.10+beta1 if this is one is not included.

@vbgl

vbgl approved these changes May 13, 2019

@vbgl vbgl modified the milestones: 8.10.0, 8.10+beta1 May 13, 2019

@coqbot coqbot added this to Request 8.10 inclusion in Coq 8.10 May 13, 2019

@vbgl vbgl merged commit 60e976a into coq:master May 13, 2019

7 checks passed

GitLab CI pipeline Pipeline completed on GitLab CI
Details
coq.coq Build #20190508.7 succeeded
Details
coq.coq (Windows) Windows succeeded
Details
coq.coq (macOS) macOS succeeded
Details
doc:ml-api:odoc: ml-api artifact Link to ml-api build artifact.
Details
doc:refman: refman artifact Link to refman build artifact.
Details
doc:refman: stdlib artifact Link to stdlib build artifact.
Details

vbgl added a commit that referenced this pull request May 13, 2019

Merge PR #10085: Do not include unreleased changelog in released vers…
…ions.

Ack-by: SkySkimmer
Ack-by: Zimmi48
Ack-by: jfehrle
Reviewed-by: vbgl

@Zimmi48 Zimmi48 deleted the Zimmi48:is-a-release branch May 13, 2019

vbgl added a commit to vbgl/coq that referenced this pull request May 13, 2019

@coqbot coqbot moved this from Request 8.10 inclusion to Shipped in 8.10+beta1 in Coq 8.10 May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.