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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various build cleanups #13112

Merged
merged 11 commits into from
May 15, 2024
Merged

Various build cleanups #13112

merged 11 commits into from
May 15, 2024

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Apr 23, 2024

This PR collects a whole load of related tweaks together which started from some checking done in #13093.

  • As I noted in Allow building with clang-cl聽#13093 (comment), the build should avoid ever using absolute paths as we end up in the Dante's missing 10th circle (escaping rules). The first commit switches the winpthreads submodule to be referred using a relative path and adds comments in configure.ac which I should have put in Overhaul the FlexDLL bootstrap聽#10135 originally.
  • The next commit then simplifies the meta-programming between configure.ac and Makefile for these two submodules - i.e. configure.ac stops generating Makefile-fragments and simply emits strings, like it's supposed to.
  • The next commit is a .gitignore entry we missed in Restore the MSVC port of OCaml聽#12954 (we also missed cleaning winpthreads-sources in distclean which is fixed a few commits later).
  • I'm not sure how long it's been wrong, but the rule for cleaning yacc/wstr.obj (a Windows-specific object) didn't work because it relies on Makefile.build_config which isn't loaded in a clean target. That's also a bit of meta-programming / layer violation between configure/make which I'm not terribly fond of, but I don't have a better solution to hand at the moment.
  • Use webman/*.html and webman/api for OCaml.org manuals聽#12976 accidentally broke the ability to run make distclean from an unconfigured tree.
  • Similarly to the yacc/wstr.obj issue, the winpthreads objects were never cleaned because the instructions relied on configuration which isn't loaded in a clean target.
  • Morally, make clean ought to clean the flexdll objects, which is done in the next commit (it's only morally because make clean does clean the flexlink executable and for reasons of history flexdll's Makefile always compiles by specifying the .ml files on the command line, rather than the more traditional separate compilation and linking, so flexlink was always completely recompiled after a make clean).
  • Finally, there's an unfortunate nit when working on the MSVC port that the winpthreads submodule ends up with untracked content because of the .obj files. I've fixed that by instead putting the winpthreads objects in runtime/winpthreads during the build.

cc @shindere, @shym, @MisterDA 馃檪

Copy link
Contributor

@shym shym left a comment

Choose a reason for hiding this comment

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

Nice clean up!

I鈥檝e pushed on my cleanups-suggest branch two small suggestions:

  • to go back to flexdll and winpthreads configuration code and remove the variables rendered nicely obsolete by this PR,
  • to drop cleaning of winpthreads submodule now that it is built out-of-tree.

dra27 and others added 11 commits May 15, 2024 09:55
In particular, this means the Makefile fragments (-I $(ROOTDIR)/flexdll
etc) do not get put into ocamltest/ocamltest_config.ml
Makefile.build_config isn't loaded for make clean
Philosophical fix - flexlink is unconditionally rebuilt from the .ml
files if the executable is missing, but make clean should actually clean
objects.
The way the winpthreads submodule has been extracted from mingw-w64
means that there is no .gitignore for objects created in it which is a
git-status nuisance when developing. Fix that by instead placing the
objects directly in the runtime/ directory.
Also removes a few obsolete variables
@dra27
Copy link
Member Author

dra27 commented May 15, 2024

Sorry for the lag. Dropping the clean statement was in my mind, but apparently I then didn't do it, so thanks for that one.

For flexdll, the $(ROOTDIR)/flexdll wasn't originally an accident, but it wasn't doing that for winpthreads ... and I decided I prefer your version anyway, so I've pulled that commit straight in, thank you!

Copy link
Contributor

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

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

Some of these things were really bugging me, thanks for the fixes!
LGTM.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

Approved on @MisterDA's behalf.

@gasche gasche merged commit 44f23e2 into ocaml:trunk May 15, 2024
15 checks passed
@dra27 dra27 deleted the cleanups branch May 15, 2024 12:25
@shindere
Copy link
Contributor

shindere commented May 15, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants