-
Notifications
You must be signed in to change notification settings - Fork 180
Simplify pickup of system protobuf #892
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
Conversation
|
@davidrohr @ktf this is what I have in mind, I am currently testing it with O2. For FairRoot, I have already tested it and it correctly picks:
This works with a patched FairRoot only. Bump to patch version will be added to this PR later. Stay tuned! |
|
See FairRootGroup/FairRoot#652, proposed for upstream dev. |
* Use a single variable pointing to the root installation * Do not pass the variable if protobuf comes from the system * Bump FairRoot to a version compatible with our fix
c2183b1 to
e6bc9f6
Compare
|
So if you pick up this alidist version (and you don't have FairRoot in devel mode, so that you are forced to use our version) it should work. Waiting for confirmation from @davidrohr :-) |
|
@dberzano : Hi Dario, I rebuilt from scratch and protobuf is picked up correctly from the system. |
|
@davidrohr thanks for checking, I had to merge it before your check as @shahor02 needed it sooner :-) |
|
@dberzano : I saw that, but wanted to let you know that it works with system protobuf as well, so I think we can close this topic now. |
|
@dberzano this (or something related to this) seems to have broken the PR builds on system which have both system and our protobuf, e.g. the build machines. See for example: |
|
@dberzano @dennisklein for the record the actual problem seems to be: which if I understand correctly is simply wrong. |
|
Yes, looks wrong. It seems to mix up system installation with custom installation somehow. I had a holiday today, will have a look at it tomorrow. |
|
I have a fix. I'm making a PR. |
Should supersede #879.