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

Pngupdate #1535

Merged
merged 6 commits into from Sep 12, 2015
Merged

Pngupdate #1535

merged 6 commits into from Sep 12, 2015

Conversation

@dnadeau4
Copy link
Contributor

@dnadeau4 dnadeau4 commented Sep 10, 2015

No description provided.

@@ -1,6 +1,6 @@
set(libcdms_source "${CMAKE_CURRENT_BINARY_DIR}/build/libcdms")
set(libcdms_install "${cdat_EXTERNALS}")
set(CONFIGURE_ARGS --srcdir=${libcdms_source}^^--enable-dap=^^--enable-drs=no^^--enable-hdf=no^^--enable-pp=yes^^--enable-ql=no^^--cache-file=/dev/null^^--prefix=${libcdms_install}^^--with-nclib=${cdat_EXTERNALS}/lib^^--with-ncinc=${cdat_EXTERNALS}/include^^--with-daplib=/lib^^--with-dapinc=/include^^--with-hdfinc=./include^^--with-hdflib=./lib^^--with-hdf5lib=${cdat_EXTERNALS}/lib^^--with-grib2lib=${cdat_EXTERNALS}/lib^^--with-jasperlib=${cdat_EXTERNALS}/lib^^--with-grib2inc=${cdat_EXTERNALS}/include^^--enable-grib2)
set(CONFIGURE_ARGS --srcdir=${libcdms_source}^^--enable-dap=^^--enable-drs=no^^--enable-hdf=no^^--enable-pp=yes^^--enable-ql=no^^--cache-file=/dev/null^^--prefix=${libcdms_install}^^--with-nclib=${cdat_EXTERNALS}/lib^^--with-ncinc=${cdat_EXTERNALS}/include^^--with-daplib=/lib^^--with-dapinc=/include^^--with-hdfinc=./include^^--with-hdflib=./lib^^--with-hdf5lib=${cdat_EXTERNALS}/lib^^--with-pnglib=/usr/X11R6/lib^^--with-grib2lib=${cdat_EXTERNALS}/lib^^--with-jasperlib=${cdat_EXTERNALS}/lib^^--with-grib2inc=${cdat_EXTERNALS}/include^^--enable-grib2)
Copy link
Contributor

@aashish24 aashish24 Sep 10, 2015

--with-pnglib=/usr/X11R6/lib

Will not work on all systems.

Copy link
Contributor

@doutriaux1 doutriaux1 Sep 10, 2015

@dnadeau4 that is probably why it fails on other systems. @aashish24 @jbeezley could we use cmake find(libpng) or similar to set this path?

Copy link
Contributor

@aashish24 aashish24 Sep 10, 2015

Yes, using our own custom module to find PNG is the best route.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Sep 10, 2015

@dnadeau4 this breaks everything, looks like cdms2 does not build anymore. We need to fix this rather quickly because the libcdms update now breaks mac in master.

@@ -403,7 +403,7 @@ if(APPLE)
set(cdat_osx_arch_flag_fortran "-m${CMAKE_OSX_ARCHITECTURES_M}")
set(cdat_osx_version_flag "-mmacosx-version-min=${CMAKE_OSX_DEPLOYMENT_TARGET}")
set(cdat_osx_sysroot "-isysroot ${CMAKE_OSX_SYSROOT}")
set(cdat_osx_cppflags "-I${CMAKE_OSX_SYSROOT}/usr/include ${cdat_osx_arch_flag} ${cdat_osx_version_flag} ${cdat_osx_sysroot} -pipe ")
set(cdat_osx_cppflags "-I${CMAKE_OSX_SYSROOT}/usr/include -I/usr/X11R6/include ${cdat_osx_arch_flag} ${cdat_osx_version_flag} ${cdat_osx_sysroot} -pipe ")
Copy link
Member

@durack1 durack1 Sep 10, 2015

On a "vanilla" OS X 10.10.5 install, the include subdir doesn't exist

Copy link
Contributor

@doutriaux1 doutriaux1 Sep 10, 2015

vanilla is gone, we need libpng now both for this and matplotlib.

Copy link
Member

@durack1 durack1 Sep 10, 2015

@doutriaux1 on the wiki there is no mention of XQuartz as a required dependency, well there is but it notes UVCDAT <v1.3.x..

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Sep 10, 2015

@doutriaux1 matplotlib should be off by default (as that's what we discussed) and therefore the xquartz should be a optional dependency. My point is that we need to keep the core dependencies as small as possible or otherwise we won't be able to attract new users.

@durack1
Copy link
Member

@durack1 durack1 commented Sep 10, 2015

@aashish24 @doutriaux1 to make a provocative comment.. Wouldn't all this dependency talk go away if a clean *.dmg installer package was generated for OS X?

Why would you bother building something if you can download a *.dmg and double click, drag into your /Applications folder and start working?

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Sep 10, 2015

ok normal test failing, but retriggering it just to make sure that I have merged @sankhesh baselines in.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Sep 11, 2015

@aashish24 matplotlib was decided to be out back on by default at the time @sankhesh put it in.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Sep 11, 2015

All right all good let's merge

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Sep 11, 2015

@aashish24 we need this for this release. Post release I have a branch to link against mac's native libpng. But it's clunky it needs to edit the c code because Apple changed the names of some function calls (added _cg prefix) . if you feel strongly about it we could also remove the X11 bit and buold png again. But that also leads to some issues if I recall correctly and that was why we decided to take it off. @dnadeau4 could try another branch in which you simply enable the build of libpng on Mac. That might be the best. Actually I will try it too next week if you don't beat me to it.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Sep 11, 2015

@doutriaux1 we cannot merge this as it is this line "--with-pnglib=/usr/X11R6/lib" which would not be valid on linux system. If anything we need to make sure that we guard against Linux. I understand we are under tight deadline but we should refrain from merging code that could break other systems 😢 : .

At miminum, please guard against the mac vs other system if not done so. I am okay if we link against system PNG after release. @jbeezley I found that you can install PNG via brew as well, I haven't tried it though.

@dnadeau4
Copy link
Contributor Author

@dnadeau4 dnadeau4 commented Sep 11, 2015

@aashish24 "--with-pnglib=/usr/X11R6/lib" won't break anything, it will only add a -L/usr/X11R6/lib which won't be used except for MAC OS. if you want a to add a platform check that is fine too.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Sep 11, 2015

@aashish24 I sort of agree with you. @dnadeau4 let's not add the oath here, but add it to APPLE search path in CMakeLists.txt also we could look for it on Mac and turn on the libpng if it is missing.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Sep 11, 2015

@aashish24 "--with-pnglib=/usr/X11R6/lib" won't break anything, it will only add a -L/usr/X11R6/lib which won't be used except for MAC OS. if you want a to add a platform check that is fine too.

@dnadeau4 yes, it might not break things but it would be odd to append a path that does not exists on a system. I am not 100% sure how it would work on supercomputers and some other linkers that we don't test on regular basis. I would suggest if you want to do minimum work, just check for apple and duplicate external project add code.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Sep 12, 2015

@dnadeau4 thanks..this is looking great. Post 2.4 we should search for PNG and build our own if not found. I will create a new issue for that.

aashish24 added a commit that referenced this issue Sep 12, 2015
@aashish24 aashish24 merged commit ff196c5 into master Sep 12, 2015
3 checks passed
@aashish24 aashish24 deleted the pngupdate branch Sep 12, 2015
@dnadeau4
Copy link
Contributor Author

@dnadeau4 dnadeau4 commented Sep 12, 2015

@aashish24 thanks! I am happy you merged it. We can work on a better solution for 3.0. Mac OS has libPng.dylib and @doutriaux1 did some work trying to use it. I'll look into it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants