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

Bump arpack-ng to 3.3.0 #14518

Merged
merged 2 commits into from
Apr 18, 2016
Merged

Bump arpack-ng to 3.3.0 #14518

merged 2 commits into from
Apr 18, 2016

Conversation

ViralBShah
Copy link
Member

No description provided.

@ViralBShah ViralBShah added the domain:building Build system, or building Julia or its dependencies label Dec 31, 2015
@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2015

Nope, see opencollab/arpack-ng#29. We should just move arpack out to a package to get rid of a fortran build dependency.

@ViralBShah
Copy link
Member Author

The issue is the lack of autotools?

@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2015

Yes. We haven't needed autoconf or automake thus far. Arpack certainly isn't worth adding more build time deps for, unless this fixes any real bugs.

@ViralBShah ViralBShah closed this Dec 31, 2015
@ViralBShah
Copy link
Member Author

I think there are some fixes we want, but I will dig deeper..

@ViralBShah
Copy link
Member Author

This is the main reason to update arpack. We can just pull in that specific patch too if required.

opencollab/arpack-ng#19

@tkelman
Copy link
Contributor

tkelman commented Jan 6, 2016

In the linked issue they said they might be willing to merge a PR that adds back the autotools files.

@ViralBShah
Copy link
Member Author

Can we figure out what to do here?

@tkelman
Copy link
Contributor

tkelman commented Feb 11, 2016

If we're getting in the habit of re-hosting our own tarballs for upstreams that aren't releasing as often or in the form we want...

@ViralBShah
Copy link
Member Author

I'd rather do that for now, and get fixes for the known bug in, and then revert to upstream in case they fix it.

@ViralBShah
Copy link
Member Author

So, fork arpack-ng in JuliaLang, add the autotools, and upload tarball?

@tkelman
Copy link
Contributor

tkelman commented Feb 11, 2016

Actually we don't even really need to fork it. Just do like I asked them to do - download their tagged version, run make dist and upload the resulting new tarball that make dist creates.

@ViralBShah
Copy link
Member Author

I don't have the upload credentials. Is this something you could set up?

@tkelman
Copy link
Contributor

tkelman commented Feb 11, 2016

Yes. Can it wait until next week?

@ViralBShah
Copy link
Member Author

Totally.

@nalimilan
Copy link
Member

Maybe ask upstream about their release plans first? They seem to be quite responsive.

@ViralBShah
Copy link
Member Author

Bump. What's the next step? Provide this upstream, and once they merge the PR, use that commit until they make a new release?

@tkelman
Copy link
Contributor

tkelman commented Feb 24, 2016

Milan has some good fixes in opencollab/arpack-ng#32 that would likely clean up some messiness in deps/Makefile regarding the symbol renaming, so we could either create a make dist tarball now off of their master if we don't mind using prereleases, or open a PR that adds back the autoconf generated files and see if they'll merge it.

@ViralBShah
Copy link
Member Author

Opening a PR makes more sense, I think.

@tkelman
Copy link
Contributor

tkelman commented Feb 26, 2016

I went with your idea of just completely ignoring arpack-ng's autotools build system. Had to rename one subroutine that arpack's autotools are skipping to avoid a double definition error, but wasn't too bad.

@ViralBShah
Copy link
Member Author

Glad that worked out. The SECOND thing has been around forever. Easy fix like you said.

@tkelman
Copy link
Contributor

tkelman commented Feb 27, 2016

This could use a few people testing it on Mac, or with ifort, or various other combinations of system blas/lapack etc. Do a make -C deps distclean-arpack && make cleanall for good measure.

@ViralBShah
Copy link
Member Author

I have recently started seeing these on os x. Any ideas how to pass the macosx min version flags to gfortran on os x?

ld: warning: object file (/var/folders/rj/njz8jnv11637flz6vgnb6bz00000gn/T//ccXGOuw1.o) was built for newer OSX version (10.11) than being linked (10.7)

@ViralBShah
Copy link
Member Author

The arnoldi tests pass on mac.

@tkelman
Copy link
Contributor

tkelman commented Feb 27, 2016

Where's that object file from, is it temporary while building arpack with this $(FC) line? It looks like we should already be adding the version-min stuff to FC here

julia/Make.inc

Line 365 in 636916e

FC += -mmacosx-version-min=10.7
but I'm not sure whether gfortran uses them properly. (USE_LIBCPP should be getting set on new enough osx versions here

julia/Make.inc

Lines 310 to 321 in 636916e

ifeq ($(OS), Darwin)
DARWINVER := $(shell uname -r | cut -b 1-2)
DARWINVER_GTE13 := $(shell expr `uname -r | cut -b 1-2` \>= 13)
ifeq ($(DARWINVER), 10) # Snow Leopard specific configuration
USEGCC := 1
USECLANG := 0
OPENBLAS_TARGET_ARCH:=NEHALEM
OPENBLAS_DYNAMIC_ARCH:=0
USE_SYSTEM_LIBUNWIND:=1
else
ifeq ($(DARWINVER_GTE13),1)
USE_LIBCPP := 1
)

@ViralBShah
Copy link
Member Author

I thought we do add the version-min stuff to FC, but gfortran doesn't seem to know about it - perhaps because it is not part of the apple toolchain.

@ViralBShah
Copy link
Member Author

I find this particular problem fixed if I do:

export MACOSX_DEPLOYMENT_TARGET=10.7

@tkelman
Copy link
Contributor

tkelman commented Feb 27, 2016

So were the autotools actually doing at least one useful thing and setting that flag or something equivalent?

@tkelman
Copy link
Contributor

tkelman commented Feb 28, 2016

So if gfortran (from where, homebrew?) isn't doing the right thing with the version-min flags, why is it creating object files that the linker thinks are built for the latest osx version? Someone with more understanding of Darwin toolchain particulars than me will have to tell us whether this will cause issues, or if we need to set that environment variable on the buildbots to make our binaries continue to work on old osx versions.

@nalimilan
Copy link
Member

Why don't we make a tarball including configure instead of working around it?

@tkelman
Copy link
Contributor

tkelman commented Feb 28, 2016

This also fixes unrelated issues where libtool fails to link to an external blas library on win64. Should have mentioned that.

@tkelman
Copy link
Contributor

tkelman commented Apr 18, 2016

Why don't we make a tarball including configure instead of working around it?

Alright, did that, so this should work the same as the existing arpack version on mac now. It doesn't fix the other issue I was having with libtool refusing to link against a blas dll that doesn't start with "lib" but I found a different workaround for that.

(edited a year later - the workaround I found at this time was temporarily making a copy of the blas dll that does start with "lib" just for the purposes of libtool being able to find and link to it, but there's an even better workaround in #21588)

@tkelman tkelman merged commit 029392f into master Apr 18, 2016
@tkelman tkelman deleted the vs/arpack branch April 18, 2016 22:56
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

3 participants