Conversation
|
This requires a change in the opam file. |
|
Let me play "grumpy old developer" once more. I feel this PR is making my life as a core OCaml developer more difficult. Is it worth my suffering? |
|
Xavier Leroy (2022/06/21 09:55 -0700):
Let me play "grumpy old developer" once more. I feel this PR is
making my life as a core OCaml developer more difficult. Is it worth
my suffering?
Your feeling may come from two things:
1. You may see things worse than they are. Once you will have generated `configure` you won't have to do anything, except if it changes and in that case `make should tell you` (not implemented yet but to come in this PR.)
2. Perhaps you see only the downside and don't see what this PR will make easier, namely that it removes the constraint on the autoconf version to use to update configure.
To that, I'd add that even if you do not personnally have to maintain
the CI scripts that make sure configure has been updated, that's a
burden for others which this PR relieves.
|
|
Kate (2022/06/21 08:07 -0700):
This requires a change in the opam file.
Good point, thanks!
This should now have been fixed.
|
|
|
|
Damien Doligez (2022/06/22 02:12 -0700):
`tools/ci/actions/runner.sh` needs to be updated too (for the
`basic-compiler` action).
I chekced and I did update it (it calls tools/autogen).
Can you see something else missing?
|
dra27
left a comment
There was a problem hiding this comment.
Back in 4.08 when we switched to autoconf, it felt like too many changes at once to be pulling in autoconf as a dev dependency. That said, it's a lot of effort that goes into checking that the committed configure script is actually correct - it may briefly take some getting used to, but I expect this will be simpler in the long-run.
There are two other changes in addition to the various comments:
tools/pre-commit-githookneedsHOOK_VERSIONbumped and then lines 87-193 deleted- The release notes need updating: on a release tag,
configureshould be removed from.gitignoreand thenconfigure committed(in order to get it into the GitHub release tarball) and then on the commit following the line should be restored.gitignoreandconfigureremoved.
| - name: configure correctly generated | ||
| run: >- | ||
| tools/ci/actions/check-configure.sh | ||
| '${{ github.ref }}' | ||
| '${{ github.event_name }}' | ||
| '${{ github.event.pull_request.base.ref }}' | ||
| '${{ github.event.pull_request.base.sha }}' | ||
| '${{ github.event.pull_request.head.ref }}' | ||
| '${{ github.event.pull_request.head.sha }}' | ||
| '${{ github.event.ref }}' | ||
| '${{ github.event.before }}' | ||
| '${{ github.event.ref }}' | ||
| '${{ github.event.after }}' | ||
| if: ${{ always() }} | ||
|
|
There was a problem hiding this comment.
#11346 proposes a replacement check to ensure it's not accidentally committed in future.
INSTALL.adoc
Outdated
|
|
||
| * A POSIX-compatible `diff` is necessary to run the test suite. | ||
|
|
||
| * GNU `autoconf`, if compiling from the git repository |
INSTALL.adoc
Outdated
| command from its root directory: | ||
|
|
||
| ./configure | ||
| tools/autogen |
There was a problem hiding this comment.
Nit: the previous command was indented 6 spaces (as the next one is)
| @echo "- In file README.win32.adoc for Windows systems." | ||
| @echo "On Unix systems, if you've just unpacked the distribution," | ||
| @echo "something like" | ||
| @echo " tools/autogen (needed only for git clones)" |
There was a problem hiding this comment.
This whole thing has been broken since 4.13 (see #11347!), so I'm hesitant suggesting, but this could be:
| @echo " tools/autogen (needed only for git clones)" | |
| ifeq "$(wildcard configure)" "" | |
| @echo " tools/autogen" | |
| endif |
and only suggest the command if it's clearly needed. This would stop the command being suggested if you extracted a release tarball and just ran make, for example.
| @@ -12,6 +12,7 @@ depends: [ | |||
| conflict-class: "ocaml-core-compiler" | |||
There was a problem hiding this comment.
GitHub won't let me add the comment in the correct place, but "conf-autoconf" should be added to the list in depends above.
|
To echo @xavierleroy from an opam perspective, this proposal would make the installation of trunk switches for users slightly more complicated, as they would also need to have the right version of autoconf installed (and a particular version, no less! #11294). The status quo seems fine to me with configure checked in, and some scripting magic in the CI that seems to work reasonably already... |
|
Anil Madhavapeddy (2022/06/22 07:17 -0700):
To echo @xavierleroy from an opam perspective, this proposal would
make the installation of trunk switches for users slightly more
complicated, as they would also need to have the right version of
autoconf installed
Yes they would need autoconf, but why do you write `the right version`?
(and a particular version, no less! #11294).
I don't understand this, sorry.
|
If |
|
@dra27 wrote:
I don't see how that follows. The problem in #11294 is that we need a particular version of autoconf in order for the generated If we simply drop the autoconf version requirement for developers, don't we now expose them to the possibility of an inconsistent local configure script vs what they might have gotten from the checked in version? And in complement, what is then generating the release tarball configure scripts such that they are reproducible vs what is tested in CI? If the I fear we are playing whac-a-mole with reproducibility here :-) |
|
We only cared about the reproducibility of With There’s an ease of development argument against doing this, sure, but I’m not at all sure there’s a trust argument! |
"Ca fait pitié" as we say in French. I feel sorry about the release manager who will have to put up with this. And I believe releases should be snapshots of the working sources, no more, no less. The more differences we introduce in releases, the more opportunities to get it wrong. |
|
On the specific issue of the release process, the pre-release, release and post-release commits are already special because we need to change the version number and thus update So there is in fact a slight decrease in the number of command that needs to be run during release since there will be no need to update |
I was thinking of your heart, a la #10173, were we to merge #11346 to guard against accidentally recommitting |
|
Florian Angeletti (2022/06/22 10:47 -0700):
On the specific issue of the release process, the pre-release, release
and post-release commits are already special because we need to change
the version number and thus update `configure` anyway. Moreover, there
is no need to change `.gitignore` to commit configure with the release
script, we can use `git commit --force ` with the finite number of
files that needs updating.
Yes, that makes a lot of sense to me and it feels better to me not to
change .gitignore for one commit.
The only drawback I oculd see to this approach being that `git status`
would not (I guess) show a clean output for release commits.
So there is in fact a slight decrease in the number of command that
needs to be run during release since there will be no need to update
`configure` during the pre and post release commit.
@xavierleroy may be interested in this bit of information.
|
|
David Allsopp (2022/06/22 04:03 -0700):
@dra27 commented on this pull request.
Back in 4.08 when we switched to autoconf, it felt like too many
changes at once to be pulling in autoconf as a dev dependency. That
said, it's a lot of effort that goes into checking that the committed
`configure` script is actually correct - it may briefly take some
getting used to, but I expect this will be simpler in the long-run.
I couln't have phrased things in a better way. Thanks.
There are two other changes in addition to the various comments:
- `tools/pre-commit-githook` needs `HOOK_VERSION` bumped and then
lines 87-193 deleted
Done so! Thanks!
- The release notes need updating: on a release tag, `configure`
should be removed from `.gitignore` and then `configure committed`
(in order to get it into the GitHub release tarball) and then on the
commit following the line should be restored `.gitignore` and
`configure` removed.
I did update the release notes, but I rather followed the line of using
`-f` and not touching `.gitignore`, because that seemed simpler to me
and also more in line with @Octachron's thoughts.
I also explained in a bit more details how to update the version number.
On that topic: wouldn't it be a good time to simply get rid of the
VERSION file?
I'd really like this to happen and would be more than happy to submit a
PR in this direction.
> - - name: configure correctly generated
- run: >-
- tools/ci/actions/check-configure.sh
- '${{ github.ref }}'
- '${{ github.event_name }}'
- '${{ github.event.pull_request.base.ref }}'
- '${{ github.event.pull_request.base.sha }}'
- '${{ github.event.pull_request.head.ref }}'
- '${{ github.event.pull_request.head.sha }}'
- '${{ github.event.ref }}'
- '${{ github.event.before }}'
- '${{ github.event.ref }}'
- '${{ github.event.after }}'
- if: ${{ always() }}
-
#11346 proposes a replacement check to ensure it's not accidentally
#committed in future.
Yes, will review.
> @@ -35,6 +35,8 @@
* A POSIX-compatible `diff` is necessary to run the test suite.
+* GNU `autoconf`, if compiling from the git repository
Nit: capital G on Git
Sure, done.
- ./configure
+ tools/autogen
Nit: the previous command was indented 6 spaces (as the next one is)
Fixed. Thanks.
> @@ -1654,6 +1654,7 @@ config.status:
@echo "- In file README.win32.adoc for Windows systems."
@echo "On Unix systems, if you've just unpacked the distribution,"
@echo "something like"
+ @echo " tools/autogen (needed only for git clones)"
This whole thing has been broken since 4.13 (see #11347!), so I'm hesitant suggesting, but this could be:
```suggestion
ifeq "$(wildcard configure)" ""
@echo " tools/autogen"
endif
```
and only suggest the command if it's clearly needed. This would stop
the command being suggested if you extracted a release tarball and
just ran `make`, for example.
I may have an even better suggestion, here. How about adding a script in
`tools/` to check that `configure` (1) exists and (2) is newer than its
prerequisites and use that script to decide wether to suggest the
`tools/autogen` command? Thus we could use the same machinery here and
when starting to compile. what do you think?
> @@ -12,6 +12,7 @@ depends: [
conflict-class: "ocaml-core-compiler"
GitHub won't let me add the comment in the correct place, but
`"conf-autoconf"` should be added to the list in `depends` above.
Done, hopefully it's correct.
Many thanks for the review!
|
Since this file is generated, storing it means either having to ensure it remaiins synchronized with the files it is generated from, or leaving it be not synchronised, which is not satisfactory.
|
Also relevant to this discussion is this comment: |
Follow-up to #11294 (comment)
Since this file is generated, storing it means either having to
ensure it remaiins synchronized with the files it is generated from,
or leaving it be not synchronised, which is not satisfactory.
This PR removes configure from the repository, but configure will still
be generated as part of the release process and will thus be in the
release tarballs.
The PR also gets rid of the GitHub actions used to verify that
configure was in sync with configure.ac.