Closed straight-shoota closed 2 years ago
bcardiff
commented
2 years ago Without the .sh wrapper how would the .tar.gz work at any path where it is expanded?
It's fine to remove the use of the wrapper in distro packages but having the .tar.gz work with just expansion (and having all the required libs) is ver comfortable.
straight-shoota
commented
2 years ago By now, the binary should work at any location without the wrapper.
What feature does the wrapper still provide that the binary doesn't after https://github.com/crystal-lang/crystal/pull/11030 ?
The tar.gz has never contained all required libraries (that's added with #102 but it will be a separate tar.gz). It only ships with our custom build of libgc. That still works and it's linked correctly (the latest commit fixes the lib path configuration).
bcardiff
commented
2 years ago What feature does the wrapper still provide that the binary doesn't after crystal-lang/crystal#11030 ?
I was missing that point. Wrapper scripts is no longer needed 👍
The tar.gz has never contained all required libraries
That's correct. (I think) I didn't imply the opposite.
If the shipped compiler was built with CRYSTAL_CONFIG_LIBRARY_PATH=$ORIGIN/../lib/crystal, how is the the next gen going to grab the gc built in this docker file. I think it will grab the one embedded in the previous compiler always. I think we will be now missing setting CRYSTAL_LIBRARY_PATH to a non empty value in order to prevent the libs from $ORIGIN/../lib/crystal getting to the next gen.
straight-shoota
commented
2 years ago (I think) I didn't imply the opposite.
:+1: I was a little confused by "having all the required libs", but I assumed you meant essentially libgc by that (currently). Just wanted to record that we don't really package all required libs, in case somebody would misinterpret that.
Good point about CRYSTAL_LIBRARY_PATH. Yeah, we might need to tweak the Makfile for that. Or the bin/crystal script from the compiler repo?
bcardiff
commented
2 years ago Having another makefile option to ignore_embedded_libs is fine probably. The implementation of that will still be to use a non existing path to override the default option that includes origin.
Whatever we choose should be announced for maintainers since they need to do something explicitly in order to avoid linking to the embedded libs.
Maybe it would be better to ignore by default the embedded libs in the compiler itself? 🤔
straight-shoota
commented
2 years ago For the record, any issues with library paths are a direct effect of https://github.com/crystal-lang/crystal/pull/11030 and not directly related to make install itself. But 9223ef8fa7214bd61ca85bcc863b6e9eca5f583f is still necessary because the location of the lib directory changes with the default baked-in CRYSTAL_LIBRARY_PATH.
Technically, we could consider resolving the lib path issue first and then switching to make install.
As far as I can see, the only thing we need to change is this:
--- c/linux/Dockerfile
+++ i/linux/Dockerfile
@@ -59,7 +59,7 @@ RUN git clone https://github.com/ivmai/bdwgc \
&& ./configure --disable-debug --disable-shared --enable-large-config \
&& make -j$(nproc) CFLAGS=-DNO_GETCONTEXT
-ENV LIBRARY_PATH=/bdwgc/.libs/
+ENV CRYSTAL_LIBRARY_PATH=/bdwgc/.libs/
RUN llvm-config --version
ARG previous_crystal_releaseThis explicitly overrides the baked-in path from CRYSTAL_CONFIG_LIBRARY_PATH.
Another path-related issue is that the deep-path to the binary is added to PATH, instead of the location of the wrapper script. This deep path no longer exists if we replace the wrapper script with the binary directly.
--- c/linux/Dockerfile
+++ i/linux/Dockerfile
@@ -64,7 +64,7 @@ RUN llvm-config --version
ARG previous_crystal_release
ADD ${previous_crystal_release} /tmp/crystal.tar.gz
-ENV PATH=${PATH}:/tmp/crystal/lib/crystal/bin/
+ENV PATH=${PATH}:/tmp/crystal/bin/
RUN mkdir -p /tmp/crystal \
&& tar xz -f /tmp/crystal.tar.gz -C /tmp/crystal --strip-component=1 \
&& crystal --version \Now, we could consider leaving the old paths (lib/crystal/bin and lib/crystal/lib) as symbolic links to the new ones (bin and lib) for a transition period. This would allow using the same instructions for packages with the old and new paths.
bcardiff
commented
2 years ago Changing CRYSTAL_LIBRARY_PATH is enough. But if the goal is to have a make install target that others can use I think it should have some parameters as a more stable API.
Regarding the PATH set in docker, having symlinks or both paths added ENV PATH=${PATH}:/tmp/crystal/lib/crystal/bin/:/tmp/crystal/bin/ should work as a transition. 👍
straight-shoota
commented
2 years ago Yeah, I think symlinks is probably a better solution because it works for anyone using the tar.gz with the previous paths.
At some point, we'll have to break that, though. But at least we can offer this for a transition period.
straight-shoota
commented
2 years ago With the last two commits, the build works for any bootstrap version with new or old library paths.
At some point we just need to remove the legacy symlinks and update PATH (the latter can be done as soon as we don't need any pre-1.2.0 release as bootstrap compiler).
straight-shoota
commented
2 years ago straight-shoota
commented
2 years ago
Since https://github.com/crystal-lang/crystal/pull/10878 was merged, we can replace the custom installation steps with
make install.shardsdid already havemake installfor a long time and we can use that as well.There are a couple of differences in the resulting tarball:
bin/instead oflib/crystal/bin. Since https://github.com/crystal-lang/crystal/pull/11030 there's no need for a wrapper script anymore. We could consider to still keep using it, but I don't think that's necessary.shardsnever required a wrapper script andbin/shardswas just a symlink tolib/crystal/bin/shards.shardsmanpages are uncompressed (should be fixed with https://github.com/crystal-lang/shards/pull/524).make installrecipe (https://github.com/crystal-lang/crystal/pull/10878#pullrequestreview-698572369). Examples should be considered part of the documentation (or a separate samples package).make install.llvm_ext.ohas been removed. This object file must not be part of the binary distribution package. It depends on the libraries of the target system and is not generically usable. See https://github.com/crystal-lang/crystal/pull/10880The omnibus script could also make use of
make install, but I don't have a mac to test that locally. So this is only for the linux tarball build.Resolves #38