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

Updated protobuf and py-protobuf to version 3.6.1 #9536

Merged
merged 3 commits into from
Nov 15, 2018

Conversation

Zehvogel
Copy link
Contributor

also updated the download link as the repository has moved

also updated the download link as the repository has moved
@@ -53,7 +54,7 @@ class Protobuf(CMakePackage):

depends_on('zlib')

conflicts('%gcc@:4.6') # Requires c++11
conflicts('%gcc@:4.6', when='@3.6.0:') # Requires c++11

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this only apply to versions >= 3.6 when it previously applied to all versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The protobuf release page states that c++11 is only required from version 3.6 onward. In particular it says

Starting from this release, we now require C++11. For those we cannot yet upgrade to C++11, we will try to keep the 3.5.x branch updated with critical bug fixes only. If you have any concerns about this, please comment on issue protocolbuffers/protobuf#2780.

I have to admit that I didn't bother to check why someone added that conflict in the first place. I have now stumbled across #3948 where the conflict was added but it doesn't really make sense to me. If the build really fails because of a gcc version < 4.6 and c++11 is the reason someone should probably inform the protobuf developers about that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the build really fails because of a gcc version ...

Can we try this before we lower the restriction? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try this before we lower the restriction? :)

I will try it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it is not really trivial to do spack install [email protected] maybe someone should rework that package as well :(
I will keep trying :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know. Installing older GCC with newer GCC releases turns out to be non-trivial, since usually one would only do it the other way around 🙈

Copy link
Contributor Author

@Zehvogel Zehvogel Oct 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with a [email protected] on a centos6 and it works for all versions of the package except 3.2.0, 3.3.0 and as expected 3.6.1 (c++11). I will modify the conflict accordingly soon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing!

@scheibelp
Copy link
Member

I will modify the conflict accordingly soon.

I'm going to tack dont-merge-yet to this. @Zehvogel once you have addressed this feel free to remove it.

@Zehvogel
Copy link
Contributor Author

I have modified the conflict :) sorry that it took so long

@Zehvogel
Copy link
Contributor Author

@scheibelp I seem to be unable to remove the don't-merge-yet label myself

@alalazo alalazo merged commit 2b64047 into spack:develop Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants