-
Notifications
You must be signed in to change notification settings - Fork 31
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
Conversation
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 |
c98405e
to
7459cfc
Compare
There was a problem hiding this 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 thewindres
tool for the resources),version.res
generated byrc
,
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.
@@ -5,14 +5,17 @@ | |||
## | |||
|
|||
|
|||
VERSION = 0.43 | |||
VERSION = \ | |||
$(eval VERSION := $$(shell sed -ne 's/^version: *"\(.*\)"/\1/p' flexdll.opam))$(VERSION) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) $< |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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" | ||
] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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.
08c092b
to
d4841d2
Compare
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
d4841d2
to
cfd82ea
Compare
There was a problem hiding this 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 @@ | |||
## | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Fetch the version number from its source, in flexdll.opam |
There was a problem hiding this comment.
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.
cfd82ea
to
c5d59b5
Compare
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! |
I'll merge this once CI catches up - thank you very much for the very careful reviewing! |
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.