Use make install for linux build
#129
Conversation
|
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. |
|
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 crystal-lang/crystal#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). |
I was missing that point. Wrapper scripts is no longer needed 👍
That's correct. (I think) I didn't imply the opposite. If the shipped compiler was built with |
👍 I was a little confused by "having all the required libs", but I assumed you meant essentially Good point about |
|
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? 🤔 |
|
For the record, any issues with library paths are a direct effect of crystal-lang/crystal#11030 and not directly related to Technically, we could consider resolving the lib path issue first and then switching to 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 Another path-related issue is that the deep-path to the binary is added to --- 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 ( |
|
Changing Regarding the PATH set in docker, having symlinks or both paths added |
|
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. |
|
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 |
straight-shoota
changed the title
make install for linux build
Since crystal-lang/crystal#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 AddCrystalPath.expand_paths, expand relative to compiler path crystal#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 Compress manpages on install shards#524).make installrecipe (Addmake installcrystal#10878 (review)). 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 Movellvm_ext.oto.buildcrystal#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