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

Make install improvements #1643

Merged
merged 2 commits into from
Feb 27, 2018
Merged

Conversation

ellert
Copy link
Contributor

@ellert ellert commented Feb 17, 2018

Don't install intermediate static libs (mathtext and minicern).
Don't add JupyROOT python extension to cmake exports.

@ellert ellert requested a review from amadio as a code owner February 17, 2018 05:14
@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@Axel-Naumann
Copy link
Member

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on centos7/gcc49, mac1013/native, slc6/gcc49, slc6/gcc62, slc6/gcc62, ubuntu16/native, ubuntu16/native, windows10/vc15 with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ubuntu16/native.
See console output.

@amadio amadio self-assigned this Feb 19, 2018
@etejedor
Copy link
Contributor

After discussing with @amadio it should be fine to add the proposed flag for JupyROOT, since it is a library only intended to be used internally from the ROOT Jupyter kernel.

@amadio
Copy link
Member

amadio commented Feb 19, 2018

Yes, this is probably OK to merge, but I will confirm with @Axel-Naumann first that minicern is not used by external users before removing it from the installation.

@Axel-Naumann
Copy link
Member

Minicern is used, see e.g. https://github.com/cjpl/mdaq-midas/blob/master/roody/Makefile

@bellenot
Copy link
Member

And maybe @couet can give feedback too, since this is related to hbook/PAW (if I remember correctly)

@ellert
Copy link
Contributor Author

ellert commented Feb 19, 2018

@Axel-Naumann: That Makefile looks for libminicern.so - and links to it if it is found. It does not link to libminicern.a. The Makefile is backward compatible with old root versions where minicern was a shared library. Now minicern is static and linked into libHbook. Linking to libHbook only is sufficient, and is what the Makefile does when there is no libminicern.so. That Makefile does NOT need libminicern.a.

@amadio
Copy link
Member

amadio commented Feb 20, 2018

Hi @ellert, I am a bit reluctant to remove minicern without any notice, but the other two changes should be fine. Even though you are right about the Makefile linked by Axel, I think that since this is a backward incompatible change, we should at least announce the removal in the development release (coming sometime soon), and then remove it only in ROOT 6.14. What do you think?

@ellert
Copy link
Contributor Author

ellert commented Feb 27, 2018

I split off the minicern change to a separate PR #1675.

@amadio
Copy link
Member

amadio commented Feb 27, 2018

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on centos7/gcc49, mac1013/native, slc6/gcc49, slc6/gcc62, slc6/gcc62, ubuntu16/native, ubuntu16/native, windows10/vc15 with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

@amadio amadio merged commit 7276bf1 into root-project:master Feb 27, 2018
@amadio
Copy link
Member

amadio commented Feb 27, 2018

I see that the build had already passed, so I just merged it. Thanks!

@phsft-bot
Copy link
Collaborator

Build failed on slc6/gcc49.
See console output.

@ellert ellert deleted the make-install-improvements branch February 27, 2018 09:45
@phsft-bot
Copy link
Collaborator

@phsft-bot
Copy link
Collaborator

@phsft-bot
Copy link
Collaborator

@phsft-bot
Copy link
Collaborator

@phsft-bot
Copy link
Collaborator

@phsft-bot
Copy link
Collaborator

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

Successfully merging this pull request may close these issues.

6 participants