Opened 4 years ago

Closed 4 years ago

Last modified 17 months ago

#22814 closed enhancement (fixed)

Update cmake to 3.8.0

Reported by: tmonteil Owned by:
Priority: major Milestone: sage-8.0
Component: packages: experimental Keywords: days86, sdl
Cc: isuruf, fbissey Merged in:
Authors: Thierry Monteil Reviewers: Travis Scrimshaw, François Bissey
Report Upstream: N/A Work issues:
Branch: 8471784 (Commits) Commit:
Dependencies: Stopgaps:

Description

Change History (34)

comment:1 Changed 4 years ago by tmonteil

  • Branch set to u/tmonteil/update_cmake_to_3_8_0

comment:2 follow-up: Changed 4 years ago by tmonteil

  • Cc isuruf fbissey added
  • Commit set to 4ded8e24cfa768a309d0c122dda0b2eb394a8eaa
  • Keywords days86 added
  • Status changed from new to needs_review

With 3.2.3:

  • on 64bit, the build was OK and self-tests lead to 1 error (CTestTestUpload)
  • on 32bit, the build failed at configure time

With 3.8.0:

  • on 64bit, the build is OK and self-tests lead to 2 errors (CTestTestUpload and RunCMake.ctest_submit), but the second test did not exist on 3.2.3.
  • on 32bit, the build is OK and 7 self-tests failed out of 442 (98% tests passed)

So, regarding GNU/Linux, the state of this package is strictly better than before.

However, i had to remove the osx10.10.patch patch since it does not apply anymore. So, if you have access to such machines, please have a look to OSX builds.


New commits:

4ded8e2#22814 : update cmake to 3.8.0

comment:3 follow-up: Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:4 in reply to: ↑ 3 Changed 4 years ago by tmonteil

Replying to tscrim:

LGTM.

Does it compile/test well on OSX ?

comment:5 Changed 4 years ago by fbissey

Tested on OS X on top of #12426, tests returned 3 errors:

        Start 190: CTestTestUpload
190/445 Test #190: CTestTestUpload ......................................***Failed  Required regular expression not found.Regex=[Upload\.xml
] 78.11 sec
        Start 349: RunCMake.ctest_submit
349/445 Test #349: RunCMake.ctest_submit ................................***Failed  388.42 sec
        Start 383: RunCMake.Framework
383/445 Test #383: RunCMake.Framework ...................................***Failed   11.32 sec

Details [CTestTestUpload]

[ERROR_MESSAGE]
   Error when uploading file: /Users/fbissey/build/sage-clang/local/var/tmp/sage/build/cmake-3.8.0/src/Tests/CTestTestUpload/Testing/20170415-2318/Build.xml

   Error message was: Failed to connect to 192.0.2.0 port 5187: Operation timed out
   Problems when submitting via HTTP

[RunCMake.ctest_submit]

[ERROR_MESSAGE]
   Error when uploading file: /Users/fbissey/build/sage-clang/local/var/tmp/sage/build/cmake-3.8.0/src/Tests/RunCMake/ctest_submit/FailDrop-https-build/Testing/20170415-2$

   Error message was: Failed to connect to 192.0.2.0 port 5187: Operation timed out
   Problems when submitting via HTTP

Looks like a pattern... [RunCMake.Framework]: OS X specific, it looks like multiple errors trying to build stuff for iDevices...

comment:6 Changed 4 years ago by fbissey

I'd say the OS X failure in framework is because we set MACOSX_DEPLOYMENT_TARGET in sage-env. When cmake tries to build executable for iOS there is an option on the command line that tells it to produce OS X stuff (x86_64) instead. Boom when you try to link.

comment:7 follow-up: Changed 4 years ago by fbissey

Yup, confirmed. Unsetting MACOSX_DEPLOYMENT_TARGET make the framework test pass. Only those two tests that probably try to contact to an internal kitware server are now failing.

comment:8 Changed 4 years ago by git

  • Commit changed from 4ded8e24cfa768a309d0c122dda0b2eb394a8eaa to eb3ae47afee781ff2e9de249baeabab07d26fa6a
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

eb3ae47#22814 : unset MACOSX_DEPLOYMENT_TARGET.

comment:9 in reply to: ↑ 7 Changed 4 years ago by tmonteil

Replying to fbissey:

Yup, confirmed. Unsetting MACOSX_DEPLOYMENT_TARGET make the framework test pass. Only those two tests that probably try to contact to an internal kitware server are now failing.

Thanks fot the investigation, does the last commit fixes that issue ?

comment:10 Changed 4 years ago by fbissey

Not what I have done or my recommendation. I am also working on silencing the other two tests. I am acting only on spkg-check

#!/usr/bin/env bash

cd src
unset MACOSX_DEPLOYMENT_TARGET
ctest -E "(CTestTestStopTime|TestUpload)"

That made the framework test pass and silenced the TestUpload one. Gentoo silence the TestUpload one too amongst other. I still have to do something about ctest_submit. Have to find the right command.

Last edited 4 years ago by fbissey (previous) (diff)

comment:11 Changed 4 years ago by fbissey

That should take care of everything on 64bit. Cannot do anything about the 32bit tests without knowing more about them

#!/usr/bin/env bash

cd src
unset MACOSX_DEPLOYMENT_TARGET
ctest --extra-verbose --output-on-failure -E "(CTestTestStopTime|TestUpload|ctest_submit)"

And I like things to be verbose, that makes debugging so much easier.

comment:12 Changed 4 years ago by git

  • Commit changed from eb3ae47afee781ff2e9de249baeabab07d26fa6a to 96cae4e257a59bba9a6f6fcb46a20cb715bec6ad

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

96cae4e#22814 : make the self tests pass + fix dependencies

comment:13 Changed 4 years ago by tmonteil

Thanks for the hints, the tests now pass on 64bit arch. I took the opportunity to add a missing dependencies file. Regarding the 32bit issues, i might have some problems with my current VM (see #22782 that i seem to be the only one to produce), so i will rebuild a fresh one to only get the meaningful errors.

comment:14 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by isuruf

Replying to tmonteil:

However, i had to remove the osx10.10.patch patch since it does not apply anymore. So, if you have access to such machines, please have a look to OSX builds.

Since sage is changing the compiler to clang on OSX, the patch is not needed at all.

Replying to tmonteil:

Thanks for the hints, the tests now pass on 64bit arch. I took the opportunity to add a missing dependencies file.

I see that curl is a sage package now. Since it was not when the previous cmake version was added, I added logic such that curl was used from OSX system and it was built from source on Linux (cmake has a bundled curl). Can you change the spkg-install such that cmake finds sage's curl package?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by fbissey

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, François Bissey
  • Status changed from needs_review to positive_review

Replying to isuruf:

Replying to tmonteil:

However, i had to remove the osx10.10.patch patch since it does not apply anymore. So, if you have access to such machines, please have a look to OSX builds.

Since sage is changing the compiler to clang on OSX, the patch is not needed at all.

Replying to tmonteil:

Thanks for the hints, the tests now pass on 64bit arch. I took the opportunity to add a missing dependencies file.

I see that curl is a sage package now. Since it was not when the previous cmake version was added, I added logic such that curl was used from OSX system and it was built from source on Linux (cmake has a bundled curl). Can you change the spkg-install such that cmake finds sage's curl package?

Thierry did put curl in the dependencies, so it is taken care of. I am adding myself as a reviewer and put it back to positive review.

comment:16 in reply to: ↑ 15 Changed 4 years ago by isuruf

Replying to fbissey:

Thierry did put curl in the dependencies, so it is taken care of. I am adding myself as a reviewer and put it back to positive review.

Yes, but on Linux, cmake builds the bundled curl right?

comment:17 Changed 4 years ago by fbissey

  • Status changed from positive_review to needs_work

You are right spkg-install needs cleaning up. The whole section for using or not system curl needs updating, most of it will have to go and be simplified. Back to need_works.

comment:18 Changed 4 years ago by fbissey

I was trying to find the origin of the 192.0.2.0 address in the cmake test suite. It is not in the source code. cmake does something weird with its test suite. At some point it links one of the sub-folders in the Tests to the Applications folder on the mac. In my case one of the App in Applications is libreoffice. That weird address is found inside libreoffice's python code....

This is just bizarre.

I'll clean spkg-install there is other stuff in there like MACOSX_DEPLOYMENT_TARGET that may have been an early attempt at fixing some stuff (or needed to be used with the patch) that has no business here.

comment:19 Changed 4 years ago by fbissey

  • Branch changed from u/tmonteil/update_cmake_to_3_8_0 to u/fbissey/update_cmake_to_3_8_0
  • Commit changed from 96cae4e257a59bba9a6f6fcb46a20cb715bec6ad to 1d18a976daf3a1bba374cb69d08e44b390924620

Isuru, can you double check my changes, add your name to the reviewers and put it to positive review?


New commits:

1d18a97Simplify spkg-install and make it always use a system (sage) provided curl

comment:20 Changed 4 years ago by fbissey

  • Status changed from needs_work to needs_review

comment:21 Changed 4 years ago by tmonteil

Sorry for the delay, i had the exact same commit, so it is fine for me, except that i let the unset MACOSX_DEPLOYMENT_TARGET, since i am not sure whether it is still useful.

Also, looking at the bootstrap script, i was wondering whether we should add xz as a dependency and add the --system-liblzma option. Does this makes sense ?

Last edited 4 years ago by tmonteil (previous) (diff)

comment:22 follow-up: Changed 4 years ago by fbissey

If we have it (and I guess it is a requirement for R nowadays) then yes go ahead.

I tested the removal of unset MACOSX_DEPLOYMENT_TARGET and I didn't observe any problem nor did I expect any. Further if we want to take cmake from experimental to optional and may be even standard in time, then it has to go. We cannot make portable binaries if it is unset.

comment:23 in reply to: ↑ 22 Changed 4 years ago by tmonteil

Replying to fbissey:

If we have it (and I guess it is a requirement for R nowadays) then yes go ahead.

OK, xz is standard spkg, i am trying this right now.

I tested the removal of unset MACOSX_DEPLOYMENT_TARGET and I didn't observe any problem nor did I expect any. Further if we want to take cmake from experimental to optional and may be even standard in time, then it has to go. We cannot make portable binaries if it is unset.

OK, great, iirc cgal (still to be packaged) and csympy rely on cmake, so it might be interesting in a near future.

comment:24 Changed 4 years ago by tmonteil

  • Branch changed from u/fbissey/update_cmake_to_3_8_0 to u/tmonteil/update_cmake_to_3_8_0

comment:25 Changed 4 years ago by git

  • Commit changed from 1d18a976daf3a1bba374cb69d08e44b390924620 to 8471784d3d245e41405cc03ffa235f9238f2bdb1

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8471784Use xz/liblzma provided by Sage

comment:26 Changed 4 years ago by tmonteil

Tests still pass with Sage's xz.

comment:27 Changed 4 years ago by fbissey

Good, and presumably

./sage -sh
ldd -r local/bin/cmake

Doesn't report anything out of place.

comment:28 Changed 4 years ago by tmonteil

I got:

	linux-vdso.so.1 (0x00007ffddb538000)
	libz.so.1 => /opt/sagemath/sage-source/local/lib/libz.so.1 (0x00007f814f4e4000)
	liblzma.so.5 => /opt/sagemath/sage-source/local/lib/liblzma.so.5 (0x00007f814f2bf000)
	libbz2.so.1 => /opt/sagemath/sage-source/local/lib/libbz2.so.1 (0x00007f814f0af000)
	libcurl.so.4 => /opt/sagemath/sage-source/local/lib/libcurl.so.4 (0x00007f814ee48000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f814ec2b000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f814ea27000)
	librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f814e81f000)
	libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f814e514000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f814e213000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f814dffd000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f814dc52000)
	libssl.so.1.0.0 => /usr/lib/x86_64-linux-gnu/libssl.so.1.0.0 (0x00007f814d9f1000)
	libcrypto.so.1.0.0 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.0.0 (0x00007f814d5f5000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f814f6fe000)

comment:29 Changed 4 years ago by fbissey

  • Status changed from needs_review to positive_review

Looks all right to me. Let's move to the next stage.

comment:30 Changed 4 years ago by vbraun

  • Branch changed from u/tmonteil/update_cmake_to_3_8_0 to 8471784d3d245e41405cc03ffa235f9238f2bdb1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:31 Changed 4 years ago by dimpase

  • Commit 8471784d3d245e41405cc03ffa235f9238f2bdb1 deleted

It perhaps makes sense to immediately go to cmake 3.8.1, released 2 weeks ago.

comment:32 follow-up: Changed 4 years ago by fbissey

Well this ticket is already merged (8.0.beta5) so it would have to go in a new ticket. But sure let's do that if you also want to push it optional.

comment:33 in reply to: ↑ 32 Changed 4 years ago by dimpase

Replying to fbissey:

Well this ticket is already merged (8.0.beta5) so it would have to go in a new ticket. But sure let's do that if you also want to push it optional.

OK, this is now #22999, and yes, let us make it optional package, indeed.

comment:34 Changed 17 months ago by tmonteil

  • Keywords sdl added
Note: See TracTickets for help on using tickets.