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

Add notes for package maintainers #25912

Merged
merged 14 commits into from
Feb 27, 2018
Merged

Add notes for package maintainers #25912

merged 14 commits into from
Feb 27, 2018

Conversation

simonbyrne
Copy link
Contributor

Fixes #25905

README.md Outdated
@@ -347,12 +347,38 @@ Julia uses the following external libraries, which are automatically downloaded
[mbedtls]: https://tls.mbed.org/
[pkg-config]: https://www.freedesktop.org/wiki/Software/pkg-config/

### Notes for distribution package maintainers

Package maintaners will typically want to make use of system libraries where possible. Please refer to the abover version requirements and notes below.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: abover

README.md Outdated

### BLAS and LAPACK

As the pupose of Julia is to be a high-performance numerical language, a multi-threaded BLAS and LAPACK, such as OpenBLAS should be used, and _not_ the reference implementations that are the default on some systems.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: pupose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

@ararslan ararslan added docs This change adds or pertains to documentation building Build system, or building Julia or its dependencies external dependencies Involves LLVM, OpenBLAS, or other linked libraries labels Feb 6, 2018
@simonbyrne
Copy link
Contributor Author

We may want to add comments for libuv and libunwind (do we use our own forks of these?)

@eli-schwartz
Copy link
Contributor

It appears that the canonical documentation for which backports julia needs, is located in this Makefile:

# Apply version-specific LLVM patches

It might be helpful to document which patches are needed for which versions, to save distro maintainers the effort of of looking up this information -- which, if the julia maintainer is different from the llvm maintainer, probably means either downloading the llvm package's sources and trying each patch in turn, or doing some in-depth exploration of the Phabricator revision history.

The most readable way to convey this would probably be to add a README.md to the directory, with a table listing the patches and which versions they apply to. For bonus points, it could link to the upstream bug report/review.
Alternatively, a small note saying to look at that Makefile might be useful. I notice that most, but not all, of the llvm patches have a header containing Differential Revision: https://reviews.llvm.org/D_____ which is super-useful, assuming that this is consistently available. If I was @xyproto filing a bugreport on the Arch bugtracker for our llvm maintainer to include patches in the llvm39 package, I would want to be able to point to the exact upstream commit as a justification.

@simonbyrne
Copy link
Contributor Author

Thanks, they are good points.

README.md Outdated
### LLVM

The most complicated dependency is LLVM, for which we require version 3.9 with some additional patches (LLVM is not backward compatible). We recommend either:
- adding the patches to the LLVM 3.9 package of the distribution (note: these are backported upstream bug fixes, _not_ Julia-specific patches: all have been contributed into upstream LLVM), or
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note the the list of patches is available at deps/llvm.mk?

Maybe

julia/deps/llvm.mk

Lines 517 to 519 in 251a501

# Independent to the llvm version add a JL prefix to the version map
# Depends on `llvm-D31524-sovers_4.0` for LLVM_VER==3.9
$(eval $(call LLVM_PATCH,llvm-symver-jlprefix)) # DO NOT REMOVE
also needs special mentione. E.g. the Julia specific version of llvm is modified so that it can be used safely with other versions of llvm (4.0 or a system 3.9). This is only true for Julia 0.7+, before that collisions between the Julia version and the system version are likely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe that note would be better if it appeared in the Makefile itself? (it does appear apart from the others)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vchuravy Would you mind adding something to that effect (since I don't quite understand these issues)?

Copy link
Member

Choose a reason for hiding this comment

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

What Valentine just said seems to conflict with the text; that sure seems like a julia-specific patch. (But does it matter if we're using distribution-packaged LLVM? We don't care about conflicts then ight?)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this patch does exactly, but we should leave packagers deal with the issue of parallel installation of multiple LLVM versions. It's kind of complex, with some hacking and lots of options to set, and it's far beyond the scope of Julia-specific patches. If that's useful, you can point at how Fedora handles this: https://src.fedoraproject.org/rpms/llvm3.9/blob/master/f/llvm3.9.spec

@simonbyrne
Copy link
Contributor Author

simonbyrne commented Feb 6, 2018

As for patches, the following don't appear to have Phabricator or patch numbers:

@StefanKarpinski
Copy link
Member

What you're at it, do you want to add back a link for awk? It's not getting linkified.

README.md Outdated
### LLVM

The most complicated dependency is LLVM, for which we require version 3.9 with some additional patches (LLVM is not backward compatible). We recommend either:
- adding the patches to the LLVM 3.9 package of the distribution (note: these are backported upstream bug fixes, _not_ Julia-specific patches: all have been contributed into upstream LLVM), or
Copy link
Member

Choose a reason for hiding this comment

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

What Valentine just said seems to conflict with the text; that sure seems like a julia-specific patch. (But does it matter if we're using distribution-packaged LLVM? We don't care about conflicts then ight?)

README.md Outdated

A complete list of patches is available in `deps/llvm.mk`, and the patches themselves are in `deps/patches/

Using an unpatched or different version of LLVM will result in errors and/or poor performance. There are flags in the Makefiles for newer LLVM versions, but support for this should be regarded as experimental and not suitable for packaging.
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand what this means; are you saying that there are flags I can set within the Makefile to say "I'm building with LLVM 3.9 without julia patches"?

README.md Outdated

### BLAS and LAPACK

As a high-performance numerical language, Julia should be linked to a multi-threaded BLAS and LAPACK, such as OpenBLAS, and _not_ the reference implementations which are the default on some systems.
Copy link
Member

Choose a reason for hiding this comment

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

Including an example here would be nice. Something like "For example, packages such as ATLAS or OpenBLAS will provide much higher performance than the default reference libblas".

@staticfloat
Copy link
Member

Clearing up the patches discussion is the only major change I'd like to see.

README.md Outdated
@@ -347,12 +348,42 @@ Julia uses the following external libraries, which are automatically downloaded
[mbedtls]: https://tls.mbed.org/
[pkg-config]: https://www.freedesktop.org/wiki/Software/pkg-config/

### Notes for distribution package maintainers

Package maintaners will typically want to make use of system libraries where possible. Please refer to the above version requirements and additional notes below.
Copy link
Contributor

Choose a reason for hiding this comment

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

maintaners ==> maintainers

README.md Outdated
- adding the patches to the LLVM 3.9 package of the distribution.
* A complete list of patches is available in `deps/llvm.mk`, and the patches themselves are in `deps/patches/`.
* The only Julia-specific patch is the lib renaming (`llvm-symver-jlprefix.patch`), which should _not_ be applied to a system LLVM.
* The remaining patches are all upstream bug fixes, and have been contributed into upstream LLVM.
Copy link
Contributor

Choose a reason for hiding this comment

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

Indents require two leading spaces, not one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

README.md Outdated
### System Provided Libraries

If you already have one or more of these packages installed on your system, you can prevent Julia from compiling duplicates of these libraries by passing `USE_SYSTEM_...=1` to `make` or adding the line to `Make.user`. The complete list of possible flags can be found in `Make.inc`.

Please be aware that this procedure is not officially supported, as it introduces additional variability into the installation and versioning of the dependencies, and is recommended only for system package maintainers. Unexpected compile errors may result, as the build system will do no further checking to ensure the proper packages are installed.

### LLVM

The most complicated dependency is LLVM, for which we require version 3.9 with some additional patches (LLVM is not backward compatible). We recommend either:
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to mention that these are upstream patches? Packagers really appreciate this kind of information.

Copy link
Contributor

Choose a reason for hiding this comment

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

ref line 379

@nalimilan
Copy link
Member

We may want to add comments for libuv and libunwind (do we use our own forks of these?)

Definitely for libuv, for which we basically use a custom fork which should be bundled in the same package as Julia since no other app will use it. Maybe add a short section similar to that for LLVM, just to explain packagers that they should not try to use the system library? For libunwind, we use the vanilla upstream release now AFAIK (though we used to ship a RC for some time before it was released).

@simonbyrne
Copy link
Contributor Author

I think this is ready to go. The LLVM patch notes should probably go in a separate PR.

README.md Outdated

Package maintainers will typically want to make use of system libraries where possible. Please refer to the above version requirements and additional notes below.

Currently community maintained packages are:
Copy link
Contributor

Choose a reason for hiding this comment

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

these were all removed as unsupported in #23484 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see.

My argument for having this is so that maintainers can look to see what others have done. Perhaps a permalink to somewhere julialang.org or a discourse page would be more useful.

Copy link
Member

Choose a reason for hiding this comment

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

What about describing them as no-longer-maintained and still mentioning them here?

Copy link
Member

Choose a reason for hiding this comment

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

They are maintained, just not by us. OTC a different bullet below mentions packages which are unmaintained.

We could move instructions for packagers to a separate file so that casual users don't find them. But honestly we're talking about README.md, not the main download page, so I'm not too concerned. Anyway people are going to use these packages, so refusing to mention them is a bit silly IMHO. It's even useful to distinguish actively maintained packages from totally outdated ones to prevent surprises.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the stance of the upstream project, that people should be using them or that how they're built can't be vouched for and may introduce problems that are strictly due to packaging? Maintaining a list of what's maintained is bound to get out of date and doesn't sound so useful. https://julialang.org/downloads/platform.html says "It is strongly recommended that the official generic linux binaries from the downloads page be used to install Julia" and "The following distribution-specific packages are community contributed. They may not use the right versions of Julia dependencies or include important patches that the official binaries ship with. In general, bug reports will only be accepted if they are reproducible on the generic linux binaries on the downloads page." so why should the readme have redundant or contradictory statements about them?

Copy link
Contributor Author

@simonbyrne simonbyrne Feb 8, 2018

Choose a reason for hiding this comment

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

I agree we should probably move them, but for different reasons: the README is a snapshot: it isn't that useful to know which packages were maintained with 0.6 when looking at the 0.7 README. I think better to move them to a maintainable place, such as a discourse wiki page, or even a subpage of https://julialang.org/downloads/, and mention that in the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which of these would people prefer?

Copy link
Contributor Author

@simonbyrne simonbyrne Feb 8, 2018

Choose a reason for hiding this comment

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

Ah I didn't see https://julialang.org/downloads/platform.html, we can just use that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's put that information on that page, with all warnings we may want to add, especially regarding the fact that distribution packages generally lag behind upstream since they only provide the lastest version which was available when the distro was released. Better acknowledge that these packages exist and inform users than ignore them and leave users on their own discovering the potential issues.

@JuliaLang JuliaLang deleted a comment from simonbyrne Feb 9, 2018
@simonbyrne simonbyrne merged commit e4234b2 into master Feb 27, 2018
@simonbyrne simonbyrne deleted the sb/package-notes branch February 27, 2018 20:51
mbauman added a commit that referenced this pull request Mar 5, 2018
…gnment

* master:
  Make stdlib tests runnable standalone. (#26242)
  fix unary-related parsing regressions caused by #26154 (#26239)
  Formatting changes to new SSA IR devdocs [ci skip]
  use medium code model on PPC
  `retry` should support the check function proposed in the docstring. (#26138)
  mention axes in docs instead of size (#26233)
  exclude more CI files from source distro (#25906)
  Describe three-valued logic in docstrings
  deprecate using the value of `.=`. fixes #25954 (#26088)
  backport change to make CodegenPrepare work with isNoopAddrSpaceCast
  optimize the python version of abs2 in the microbenchmarks (#26223)
  Add notes for package maintainers (#25912)
  typo
  Fix broken links to functions in the manual (#26171)
  [NewOptimizer] Track inlining info
  Begin work on new optimizer framework
  add patch to make GC address spaces work on PPC
  also backport sover patch to LLVM 4.0
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 external dependencies Involves LLVM, OpenBLAS, or other linked libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants