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

Remove mention of Intel compilers and caution against using SYSTEM libraries #26432

Merged
merged 1 commit into from
Mar 16, 2018

Conversation

ViralBShah
Copy link
Member

No description provided.

@ararslan ararslan added docs This change adds or pertains to documentation building Build system, or building Julia or its dependencies labels Mar 12, 2018
@ararslan
Copy link
Member

Note to passers by: see discussion in #23407

@ararslan
Copy link
Member

Perhaps it would be worth adding the warnings against USE_SYSTEM_* to the README?

@ViralBShah
Copy link
Member Author

I guess we can kill all the CI stuff, if it is choking other work.

@ViralBShah ViralBShah changed the title Remove mention of Intel compilers. Remove mention of Intel compilers and caution against using SYSTEM libraries Mar 12, 2018
README.md Outdated
@@ -199,6 +199,7 @@ Julia does not install anything outside the directory it was cloned into. Julia
* GCC version 4.7 or later is required to build Julia.
* To use external shared libraries not in the system library search path, set `USE_SYSTEM_XXX=1` and `LDFLAGS=-Wl,-rpath,/path/to/dir/contains/libXXX.so` in `Make.user`.
* Instead of setting `LDFLAGS`, putting the library directory into the environment variable `LD_LIBRARY_PATH` (at both compile and run time) also works.
* It is important to note that the `USE_SYSTEM_*` flags should be used with caution. These are meant only for troubleshooting and porting, not for production use. Issues arising from the use of these flags will generally not be accepted.
Copy link
Member

Choose a reason for hiding this comment

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

"not for production use" could imply that distribution packages using these flags would not work, which is contradictory with guidelines for packagers given elsewhere. Maybe "not for production use by most users"? Could also say "troubleshooting, porting and packaging".

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "It is important to note that" is redundant; "should be used with caution" ought to convey the necessary gravity of this remark. Besides, going straight to the point is more effective in terms of both taking in and retaining information.

Copy link
Member Author

@ViralBShah ViralBShah Mar 14, 2018

Choose a reason for hiding this comment

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

I believe our view is very clearly that we do not recommend using SYSTEM llvm and various other packages with known bugs. When people discover those bugs because they compiled with SYSTEM libraries themselves or used packages created in such a way by others, they report them only to be told that it is unsupported. I think that is a bad experience.

If distro packagers are going to continue ignoring our llvm patches, then we will only increase our workload with issues. I think the real issue is llvm. The others we can deal with. Maybe we do not allow llvm as a dependency, and should just statically link it into libjulia - but that is a different discussion.

Copy link
Member

Choose a reason for hiding this comment

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

See recent discussion at #25912. Note that README now says:

Package maintainers will typically want to make use of system libraries where possible. Please refer to the above version requirements and additional notes below. A list of maintained Julia packages for various platforms is available at https://julialang.org/downloads/platform.html.

That's why I suggest that we mention distribution packagers here for consistency.

(FWIW, I think the situation of distro packages is slowly improving, e.g. Arch has recently started to use our LLVM patches and IIRC they have also switched to OpenBLAS.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. Good points. Let me rework that a bit.

README.md Outdated
@@ -250,7 +251,7 @@ The remaining build tools are available from the Ports Collection, and can be in
To build Julia, simply run `gmake`.
(Note that `gmake` must be used rather than `make`, since `make` on FreeBSD corresponds to the incompatible BSD Make rather than GNU Make.)

It's important to note that the `USE_SYSTEM_*` flags should be used with caution on FreeBSD.
It is important to note that the `USE_SYSTEM_*` flags should be used with caution on FreeBSD.
Copy link
Contributor

@waldyrious waldyrious Mar 13, 2018

Choose a reason for hiding this comment

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

Same nit as above. In fact, isn't this kind of redundant now? I would at least prepend this with "As mentioned above..."

Copy link
Member

Choose a reason for hiding this comment

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

It's especially important on FreeBSD, because while you may run into some issues using USE_SYSTEM_* on other platforms, it's likely that your Julia build just straight up won't work if you do it on FreeBSD.

Copy link
Member

Choose a reason for hiding this comment

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

"As mentioned above" seems okay to me.

Add a clear message against using SYSTEM versions of libraries.
Remove the SuiteSparse section, which was mainly for old versions
of SuiteSparse. We also do not encourage folks to use SYSTEM SuiteSparse.
Add a word of caution about SYSTEM flags to the README.
@ViralBShah ViralBShah merged commit 71cb0dc into master Mar 16, 2018
@ViralBShah ViralBShah deleted the vs/icc branch March 16, 2018 16:34
@ViralBShah
Copy link
Member Author

Hopefully this addresses all comments. Happy to edit further as necessary.

@@ -377,15 +378,7 @@ Julia uses a custom fork of libuv. It is a small dependency, and can be safely b

As a high-performance numerical language, Julia should be linked to a multi-threaded BLAS and LAPACK, such as OpenBLAS or ATLAS, which will provide much better performance than the reference `libblas` implementations which may be default on some systems.

### SuiteSparse

SuiteSparse is a special case, since it is typically only installed as a static library, while `USE_SYSTEM_SUITESPARSE=1` requires that it is a shared library. Running the script `contrib/repackage_system_suitesparse4.make` will copy your static system SuiteSparse installation into the shared library format required by Julia. `make USE_SYSTEM_SUITESPARSE=1` will then use the SuiteSparse that has been copied into Julia's directory, but will not build a new SuiteSparse library from scratch.
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea why this script was needed, but it still exists. I'd say we should either remove the script and its documentation, or keep both?

Otherwise, looks good to me.

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'll remove the script. Not necessary any more.

Copy link
Member Author

Choose a reason for hiding this comment

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

Clearly, it was necessary for the mac travis!

Copy link
Member

Choose a reason for hiding this comment

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

So is it also needed for Mac users in general?

Copy link
Member

Choose a reason for hiding this comment

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

No, it's just for those who want to package Julia with their system package manager, but can't just fix their system package manager (aka pretty much just our CI). Oh, and also, pretty much just limited to macOS (the default compiler flags on most linux system break this script).

@ararslan
Copy link
Member

Thanks Viral!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants