Improve the way ignored files are managed by shindere · Pull Request #11617 · ocaml/ocaml

shindere · 2022-10-11T15:10:18Z

This is a follow-up to #11346, whose idea is to avoid having files
that are both listed in .gitignore and still presentin the repository.

At the moment, there is one file in this situation in the repository,
namely tests/no-alias-deps/b.cmi. Thus this PR starts with a commit that
renames it to tests/no-alias-deps/b.cmi.in and which is borrowed from
#11346.

The present PR continues with a bit of cleanup in the root .gitignore
and finishes by merging the remaining .gitignore files present in
subdirectories into the root one, which subsumes what has been done
in #11346.

However, #11346 contains a CI test to make sure the property of no ignored
file committed is verified, which is not part of this PR.

Natural reviewers would be @dra27 and @Octachron, who may wnat to
check that all the ignored patterns for the manual are still necessary.

Octachron · 2022-10-12T08:37:39Z

.gitignore

+/manual/tools/htmlgen
+/manual/tools/htmlquote
+/manual/tools/latexscan.ml
+/manual/tools/dvi2txt


The only remaining tools in manual/tools are: transf and texquote2. Thus:

/manual/tools/transf.ml /manual/tools/texquote2 /manual/tools/transf

should be enough.

I presume that the *.dSYM pattern which was in manual/tools/.gitignore doesn't matter (the pattern at the top of the file is *.out.dSYM, but I don't know what this pattern is for - it's slightly suspicious that we don't appear to clean these files ever)

Octachron · 2022-10-12T08:52:30Z

.gitignore

+/manual/src/infoman/ocaml.hocaml.kwd
+/manual/src/library/*.tex
+/manual/src/library/*.htex
+/manual/src/library/arithstatus.mli


The arithstatus.ml file is a very old import from the csv ignore file which is no longer relevant.

dra27

There's an unintended consequence that because manual/src/textman now contains no files, git of course deletes the directory so the build of the manual is failing in CI! I know we used to leave dummy files in order to keep git creating directories, but I think we've moved away from doing that (compilerlibs/Makefile.compiler-libs, etc.) - $(MKDIR) textman added to the text: recipe in manual/src/Makefile should fix it.

dra27 · 2022-10-11T19:38:33Z

.gitignore

@@ -228,7 +301,6 @@ META
 /tools/primreq
 /tools/primreq.opt
 /tools/ocamldumpobj


I think we found tools/ocamldumpobj isn't built anymore, either?

dra27 · 2022-10-12T08:55:07Z

.gitignore

+/manual/tools/htmlgen
+/manual/tools/htmlquote
+/manual/tools/latexscan.ml
+/manual/tools/dvi2txt


I presume that the *.dSYM pattern which was in manual/tools/.gitignore doesn't matter (the pattern at the top of the file is *.out.dSYM, but I don't know what this pattern is for - it's slightly suspicious that we don't appear to clean these files ever)

shindere · 2022-10-12T09:20:21Z

Florian Angeletti (2022/10/12 01:37 -0700):

@Octachron commented on this pull request. > /manual/src/warnings-help.etex +/manual/tests/cross-reference-checker +/manual/tools/transf.ml +/manual/tools/texquote2 +/manual/tools/transf +/manual/tools/htmlgen +/manual/tools/htmlquote +/manual/tools/latexscan.ml +/manual/tools/dvi2txt The only remaining tools in `manual/tools` are: transf and texquote2. Thus: ``` /manual/tools/transf.ml /manual/tools/texquote2 /manual/tools/transf ``` should be enough.

Fixed, thanks!

shindere · 2022-10-12T09:22:32Z

Florian Angeletti (2022/10/12 01:52 -0700):

The `arithstatus.ml` file is a very old import from the csv ignore file which is no longer relevant.

Addressed, thanks.

shindere · 2022-10-12T09:39:42Z

David Allsopp (2022/10/12 02:03 -0700):

@dra27 commented on this pull request. There's an unintended consequence that because `manual/src/textman` now contains no files, git of course deletes the directory so the build of the manual is failing in CI!

Oops, sorry, wasn't careful enough.

I know we used to leave dummy files in order to keep git creating directories, but I think we've moved away from doing that

Which was a good idea, yes.

(`compilerlibs/Makefile.compiler-libs`, etc.) - `$(MKDIR) textman` added to the `text:` recipe in `manual/src/Makefile` should fix it.

Done, in a slightly different way, thanks. Also, one question for you @Octachron, please. In `manual/src/Makefile`, one reads: ``` all: $(MAKE) html $(MAKE) text $(MAKE) info $(MAKE) manual $(MAKE) index $(MAKE) manual ``` Is it intended that the different formats of the manual are built in such a sequential way? Could we replace this code by the more concise ``` all: html text info manual index $(MAKE) manual ``` Or something similar, to save recursive invocations of make which seem unnecessary and also gain in parallelism?

> @@ -228,7 +301,6 @@ META /tools/primreq /tools/primreq.opt /tools/ocamldumpobj I think we found `tools/ocamldumpobj` isn't built anymore, either?

Yes indeed, it was you who ofund that out, many thanks, I amended.

> /manual/src/warnings-help.etex +/manual/tests/cross-reference-checker +/manual/tools/transf.ml +/manual/tools/texquote2 +/manual/tools/transf +/manual/tools/htmlgen +/manual/tools/htmlquote +/manual/tools/latexscan.ml +/manual/tools/dvi2txt I presume that the `*.dSYM` pattern which was in `manual/tools/.gitignore` doesn't matter (the pattern at the top of the file is `*.out.dSYM`, but I don't know what this pattern is for - it's slightly suspicious that we don't appear to clean these files ever)

I thought the pattern in the root file was subsuming that one but I was wrong, sorry about that. My guess was that those `.dSYM` files were created on MacOS but you are right that if they indeed are, it would make sense to remove them at least during make distclean. So I'd say, either they are no longer created and we should stop ignoring them, or they still are and we should clean them in addition to ignoring them. @damiendoligez or @garrigue, perhaps you know?

Octachron · 2022-10-12T12:15:18Z

Concerning the all target in the manual, the index target has a hidden dependency on the manual target but otherwise the other targets can be parallelized.

shindere · 2022-10-12T14:44:20Z

Florian Angeletti (2022/10/12 05:15 -0700):

Concerning the `all` target in the manual, the `index` target has a hidden dependency on the `manual` target

OK.

but otherwise the other targets can be parallelized.

Implemented in this PR, because it didn't feel worht a dedicated one, if that's okay.

The file in question is both supposed to be ignored (as all the .cmi files) and still present in the repository. This commit renames it so that there is no file left in this situation.

These tools do no longer exist

As a consequence of the previous commit, these directories became empty and hence get removed by git. They must therefore be created during the build and removed during cleanup.

shindere · 2022-10-13T19:13:05Z

CI okay.

I have withdrawn theparallelization attempt because it can not be made
work easily and it's out of the scope of this PR anyway.

Basically, the obstacle is that several recipes create files with
similar names so there is a filename collision which would need to be
fixed.

Anyway: this PR should now be ready for review and hopefuly even
for approval and merge. :-)

Autoconf 2.71 saves an existing configure script to configure~ before generating a new one. Althogh tools/autogen could delete configure~, it may be helpful to not remove it so that it's possible to compare the two versions of the script and make sure no unintended change gets in. So this commit adds configure~ to .gitignore and makes sure it gets deleted during make clean.

shindere · 2022-10-14T10:48:58Z

I just added a commit to this PR to deal with the configure~ file created
by autoconf 2.71.

The file is kept to make it eaiser to see the differences between the
former configure script and the just generated one, ignored to avoid
noise in git status and deleted during make clean.

dra27

LGTM!

shindere · 2022-10-14T11:57:13Z

Cool! Thaks!

Improve the way ignored files are managed (cherry picked from commit 7eefbf8)

shindere added the no-change-entry-needed label Oct 11, 2022

Octachron reviewed Oct 12, 2022

View reviewed changes

dra27 reviewed Oct 12, 2022

View reviewed changes

shindere force-pushed the gitignore branch from f185d2c to 17741af Compare October 12, 2022 09:19

shindere force-pushed the gitignore branch 3 times, most recently from 6b1d5b9 to 87e9030 Compare October 13, 2022 12:39

dra27 and others added 5 commits October 13, 2022 14:46

testsuite: rename tests/no-alias-deps/{b.cmi => b.cmi.in}

88820af

The file in question is both supposed to be ignored (as all the .cmi files) and still present in the repository. This commit renames it so that there is no file left in this situation.

Stop ignoring tools/dumpapprox and tools/keywords

d112807

These tools do no longer exist

Merge the remaining .gitignore files into the root one

aa0de73

In .gitignore, stop ignoring no longer existing manual-related tools

00b9167

Stop ignoring arithstatus.mli in manual, it no longer exists

6de9db2

shindere force-pushed the gitignore branch from 87e9030 to 97bda37 Compare October 13, 2022 12:46

shindere added 2 commits October 13, 2022 15:40

Manual: make sure the infoman, texstuff and textman directories exist

7b79269

As a consequence of the previous commit, these directories became empty and hence get removed by git. They must therefore be created during the build and removed during cleanup.

Stop ignoring tools/ocamldumpobj, not built any longer

40205a2

shindere force-pushed the gitignore branch from 97bda37 to 40205a2 Compare October 13, 2022 13:43

dra27 approved these changes Oct 14, 2022

View reviewed changes

shindere merged commit 7eefbf8 into ocaml:trunk Oct 14, 2022

shindere deleted the gitignore branch October 14, 2022 12:13

dra27 mentioned this pull request Oct 14, 2022

CI check for committing files in .gitignore #11346

Merged

dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 21, 2023

Merge pull request ocaml#11617 from shindere/gitignore

8265f6a

Improve the way ignored files are managed (cherry picked from commit 7eefbf8)

dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 21, 2023

Merge pull request ocaml#11617 from shindere/gitignore

9d384f7

Improve the way ignored files are managed (cherry picked from commit 7eefbf8)

ejgallego pushed a commit to ejgallego/ocaml that referenced this pull request Nov 5, 2025

Merge pull request ocaml#11617 from shindere/gitignore

5a6a645

Improve the way ignored files are managed (cherry picked from commit 7eefbf8)

Conversation

shindere commented Oct 11, 2022

Uh oh!

Octachron Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

dra27 Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

Octachron Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

dra27 Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

dra27 Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

shindere commented Oct 12, 2022 via email

Uh oh!

shindere commented Oct 12, 2022 via email

Uh oh!

shindere commented Oct 12, 2022 via email

Uh oh!

Octachron commented Oct 12, 2022

Uh oh!

shindere commented Oct 12, 2022 via email

Uh oh!

shindere commented Oct 13, 2022

Uh oh!

shindere commented Oct 14, 2022

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

shindere commented Oct 14, 2022 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants