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

opam package for flexdll sources #135

Merged
merged 3 commits into from
Mar 15, 2024
Merged

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Mar 13, 2024

This is the first commit from #111, split into two sub-commits for slightly easier reviewing. This deals with OCaml 4.03+, which is all that's needed for the first batch of opam-repository updates.

@dra27
Copy link
Member Author

dra27 commented Mar 13, 2024

The opam package can in fact be tested from Linux with:

$ opam pin add git+https://github.com/dra27/flexdll.git#flexdll-source-packaging
...
$ ls -l $(opam var flexdll:share)
# All the source files needed for flexdll bootstrapping

@dra27 dra27 force-pushed the flexdll-source-packaging branch 2 times, most recently from c98405e to 7459cfc Compare March 13, 2024 21:14
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.

LGTM, though I’m not sure about resource formats.
To try and ensure that the version numbers are correctly set up, I wanted to look at the resources in the binary files. I’m not sure about the proper way to do that, so I’ve opened in Visual Studio:

  • flexlink.exe generated by the MinGW toolchain (so the windres tool for the resources),
  • version.res generated by rc,

and they show the same version numbers. On the other hand, the byte sequences in the files version_res.o and version.res are different: in version_res.o, FileVersion is really encoded as a string, but it is not in version.res. I don’t know if that really matters.

Makefile Show resolved Hide resolved
@@ -5,14 +5,17 @@
##


VERSION = 0.43
VERSION = \
$(eval VERSION := $$(shell sed -ne 's/^version: *"\(.*\)"/\1/p' flexdll.opam))$(VERSION)
Copy link
Contributor

Choose a reason for hiding this comment

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

For my education, what does the extra eval layer bring here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a GNU make pattern for lazy variables. The problem with VERSION := $(shell ...) is that it gets run every time the Makefile is loaded and with VERSION = $(shell ...) the problem is it gets run every time $(VERSION) is used.

The first time $(VERSION) is expanded, it evaluates VERSION := $(shell ...) meaning that $(VERSION) is now the result of the $(shell ...) call. However, := doesn't expand to anything, so the final $(VERSION) ensures that the expansion actually returns something. Future expansions of $(VERSION) are now constants.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for all the details!
I had thought about laziness so I had tested VERSION = $(shell ... but I had called it only once 😄

-D FLEXDLL_FULL_VERSION=\\\"$(FLEXDLL_FULL_VERSION)\\\"

version.res: version.rc flexdll.opam
$(RES_PREFIX) rc /nologo $(RC_FLAGS) $<
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not due to this PR but I find it a bit confusing to have *_PREFIX variable names both for cases when they are to be glued to the actual commands (CYGWIN64_PREFIX say) and cases when they are not (MSVC64_PREFIX and, here, RES_PREFIX).
Maybe the not-to-be-glued variables could be named *_EXTRA_ENV or they could add the needed space explicitly (ie end up with ... $(EMPTY)) so they are glued when used?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, that would look much clearer (or perhaps "less unclear"!)

"flexdll_initer.c"
"reloc.ml"
"version.rc"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! late notice: shouldn’t there be flexdll.opam in there, since we’ll now need it to get the version number?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Confession: I didn’t try to build an OCaml compiler with these sources :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

😱 Eek - I hadn't either, I'm very glad you spotted this!

C:\Devel>opam pin add flexdll git+https://github.com/dra27/flexdll.git#flexdll-source-packaging
[flexdll.0.43] synchronised (git+https://github.com/dra27/flexdll.git#flexdll-source-packaging)
flexdll is now pinned to git+https://github.com/dra27/flexdll.git#flexdll-source-packaging (version 0.43)

The following actions will be performed:
=== recompile 6 packages
  ↻ base-domains        base          [uses ocaml]
  ↻ base-nnp            base          [uses base-domains]
  ↻ flexdll             0.43 (pinned)
  ↻ ocaml               5.1.1         [uses ocaml-base-compiler]
  ↻ ocaml-base-compiler 5.1.1         [uses flexdll]
  ↻ ocaml-config        3             [uses ocaml-base-compiler]

Proceed with ↻ 6 recompilations? [y/n] y

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><>  🐫
⊘ removed   base-nnp.base
⊘ removed   base-domains.base
⊘ removed   ocaml.5.1.1
⊘ removed   ocaml-config.3
⬇ retrieved ocaml-base-compiler.5.1.1  (cached)
⊘ removed   ocaml-base-compiler.5.1.1
⊘ removed   flexdll.0.43
∗ installed flexdll.0.43
[ERROR] The compilation of ocaml-base-compiler.5.1.1 failed at "make -j19".

#=== ERROR while compiling ocaml-base-compiler.5.1.1 ==========================#
# context     2.2.0~beta2~dev | win32/x86_64 |  | git+file://C:/Devel/opam-repository-3#windows-initial
# path        ~\AppData\Local\opam\test-5.1.1\.opam-switch\build\ocaml-base-compiler.5.1.1
# command     ~\AppData\Local\opam\.cygwin\root\bin\make.exe -j19
# exit-code   2
# env-file    ~\AppData\Local\opam\log\ocaml-base-compiler-42868-728b84.env
# output-file ~\AppData\Local\opam\log\ocaml-base-compiler-42868-728b84.out
### output ###
# [...]
# /usr/bin/make -C flexdll-sources MSVC_DETECT=0 OCAML_CONFIG_FILE=../Makefile.config CHAINS=mingw64 ROOTDIR=.. \
#   OCAMLRUN='$(ROOTDIR)/boot/ocamlruns.exe' NATDYNLINK=false \
#   OCAMLOPT='$(OCAMLRUN) $(ROOTDIR)/boot/ocamlc -use-prims ../runtime/primitives -nostdlib -I ../stdlib' \
#   -B flexlink.exe support
# make[3]: Entering directory '/cygdrive/c/Users/DRA/AppData/Local/opam/test-5.1.1/.opam-switch/build/ocaml-base-compiler.5.1.1/flexdll-sources'
# make[3]: *** No rule to make target 'flexdll.opam', needed by 'version.ml'.  Stop.

@dra27 dra27 force-pushed the flexdll-source-packaging branch 2 times, most recently from 08c092b to d4841d2 Compare March 14, 2024 17:55
@dra27
Copy link
Member Author

dra27 commented Mar 14, 2024

Thanks, @shym! I've re-ordered the commits to better reflect what's actually happening (and what I had been testing)... the .install file is correct for FlexDLL 0.43, so I've moved the bit to harmonise the version number to the last commit (which will ultimately be FlexDLL 0.44).

This file can be referenced from the opam packages for these
pre-releases to be used in addition to the tarball.
The flexdll package is intended for use when building OCaml in
"bootstrapped" flexdll mode and installs the required source files to
the share folder in an opam switch.

The .install can be used with both FlexDLL 0.42 and FlexDLL 0.43
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.

I went through the changes again, this is good to merge I think.
Maybe we could add a comment (for our future selves 😅) that the version number is now only stored in flexdll.opam.

@@ -5,14 +5,17 @@
##


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Fetch the version number from its source, in flexdll.opam

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

The VERSION macro in Makefile is now the single source of the version of
FlexDLL. This is used when compiling version.rc.

version.rc is corrected to adopt Windows version format, so the current
release is rendered as 0.43.0.0 instead of 0.0.0.43.

flexdll.opam is now the primary source for the version number for the
package, and both version.ml and version.rc are compiled using the
number taken from there.
@dra27
Copy link
Member Author

dra27 commented Mar 15, 2024

Out of an abundance of caution brought on by that error in the .install, I have tested the opam packaging of 4.14.1 with 0.36-0.43 (which use tarballs, and grab the .install files from these commits) and then pinned this branch as well! Out of even more caution, I quickly upgraded ocaml-base-compiler.4.05.0 and confirmed that the 0.35 source package works too!

@dra27
Copy link
Member Author

dra27 commented Mar 15, 2024

I'll merge this once CI catches up - thank you very much for the very careful reviewing!

@dra27 dra27 merged commit a60821e into ocaml:master Mar 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants