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

Update CSL #40287

Merged
merged 2 commits into from
Apr 1, 2021
Merged

Update CSL #40287

merged 2 commits into from
Apr 1, 2021

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Apr 1, 2021

No description provided.

@vtjnash vtjnash added the domain:building Build system, or building Julia or its dependencies label Apr 1, 2021
@vtjnash vtjnash requested a review from staticfloat April 1, 2021 03:17
contrib/refresh_checksums.mk Show resolved Hide resolved
contrib/refresh_checksums.mk Show resolved Hide resolved
deps/Makefile Show resolved Hide resolved
@staticfloat
Copy link
Sponsor Member

macOS says:

make[1]: *** No rule to make target `version-check-llvmunwind', needed by `version-check'.  Stop.

This adds the `.so` files, particularly for libatomic that we need on FreeBSD
…talled

Most commonly csl, llvm-tools, and clang, but there were other ones in
the wrong lists also.
Makefile Show resolved Hide resolved
@@ -472,7 +472,7 @@ source-dist:
# Make tarball with Julia code plus all dependencies
full-source-dist: light-source-dist.tmp
# Get all the dependencies downloaded
@$(MAKE) -C deps getall NO_GIT=1
@$(MAKE) -C deps getall DEPS_GIT=0 USE_BINARYBUILDER=0
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We specify USE_BINARYBUILDER=1 and USE_BINARYBUILDER=0 to generate two different kinds of full-source-dist tarballs. You shouldn't set it here unconditionally.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I think you misunderstand the target definitions here. The make -C get target would be what is used to grab all of the binary files needed to build on this machine. The getall target grabs all of the source files needed to build on any machine. The getall target therefore logically cannot be used with USE_BINARYBUILDER=1. I could add $(MAKE) -f contrib/refresh_checksums.mk here though, conditional on USE_BINARYBUILDER=1, if you like.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We have customers that are using things like make -C deps getall USE_BINARYBUILDER=1 USE_BINARYBUILDER_XXX=0 to generate a tarball that they can deploy to CI and patch with their own patches to deps and such without having to rebuild LLVM, for instance. What would you suggest they use instead?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

They should use get (or perhaps extract), which is the first stage of the build pipeline (or the second, which also supports adding custom patches after). The getall target is not part of the build pipeline, but is instead part of the uninstall / cleanall set, which are a list of all possible targets, not just those that currently need to be built locally.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

okay; once this is merged let's double-check that the generated tarballs look right.

@staticfloat staticfloat merged commit 82dc402 into master Apr 1, 2021
@staticfloat staticfloat deleted the jn/csl-update branch April 1, 2021 23:25
@staticfloat
Copy link
Sponsor Member

Update; this is not working as intended. The "with binarybuilder" tarball is bundling the source versions of dependencies. :/

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Apr 2, 2021

So you want us to add a call to refresh_checksums?

@staticfloat
Copy link
Sponsor Member

I don't think this has anything to do with refresh_checksums; when running make full-source-dist USE_BINARYBUILDER=1, we should not download the source distributions of dependencies, we should download the binary builder versions of the dependencies.

@staticfloat
Copy link
Sponsor Member

Bump; this still needs to be fixed.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Apr 14, 2021

It was unclear what you wanted. Do you want a lite-source-dist to include binaries? Do you want a full-binarysouces-dist that had some binaries and some sources? Do you just want install dist to provide binaries?

@staticfloat
Copy link
Sponsor Member

The old behavior was for make full-source-dist USE_BINARYBUILDER=1 to create a tarball that contained the source of Julia, and the binaries of all dependents. If you wanted the source of Julia and the source of dependents, you had to run make full-source-dist USE_BINARYBUILDER=0. I don't care about the names, I just want the ability to create both of these, so that we can continue to publish tarballs that have, for instance, the source of Julia and the binaries of everything else. It was kind of nice that you could generate tarballs with specific source/binary configurations (via e.g. USE_BINARYBUILDER=0 USE_BINARYBUILDER_LLVM=1). This is primarily useful for users that carry local patches against certain deps/Julia itself, but don't want to build LLVM every time.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Apr 15, 2021

It sounds like you want either make -C deps get && make full-source-dist or make -f contrib/refresh_checksums.mk && make full-source-dist?

@staticfloat
Copy link
Sponsor Member

I think you're trying to use make -f contrib/refresh_checksums.mk as a proxy for downloading the tarballs, but that's not the behavior we had before; that will download for all platforms. We want to only download for the current platform.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Apr 15, 2021

make -C deps getall never downloaded only for the current platform (hence "all"), which is why I still think you wanted make -C deps get

@staticfloat
Copy link
Sponsor Member

make -C deps getall never downloaded only for the current platform (hence "all"),

I just tested this, and it's exactly what it used to do. Perhaps it's not what was intended, but it's what happens. Just do a git checkout v1.6.0 and run make -C deps getall.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Apr 15, 2021

Ah, yes, checking though the version history, it seems that was a regression in v1.5 relative to when I added it in 01654af. Whereas, the description in the Makefile has rather cryptically said (since e85a1a5):

# additionally all targets should be listed in the getall

where, by observation of the list, "all targets" was not conditional

ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
* deps: upgrade CSL

This adds the `.so` files, particularly for libatomic that we need on FreeBSD

* build: define uninstall targets even for deps that are not always installed

Most commonly csl, llvm-tools, and clang, but there were other ones in
the wrong lists also.
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
* deps: upgrade CSL

This adds the `.so` files, particularly for libatomic that we need on FreeBSD

* build: define uninstall targets even for deps that are not always installed

Most commonly csl, llvm-tools, and clang, but there were other ones in
the wrong lists also.
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
* deps: upgrade CSL

This adds the `.so` files, particularly for libatomic that we need on FreeBSD

* build: define uninstall targets even for deps that are not always installed

Most commonly csl, llvm-tools, and clang, but there were other ones in
the wrong lists also.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:building Build system, or building Julia or its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants