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

3rd-party: rename libevent release to stable_ompi #12501

Closed
wants to merge 1 commit into from

Conversation

wenduwan
Copy link
Contributor

rpmbuild now does not accept '-' in release name.
This patch rebuilds libevent 2.1.12-stable with:

  • autoconf 2.27
  • automake 1.16.5
  • libtool 2.4.7

@wenduwan
Copy link
Contributor Author

I hit another issue with this libevent release though. It does not work with newer rpmbuild due to ambiguous python shebang because of this line. I had to manually change it to #!/usr/bin/env python3

@jsquyres
Copy link
Member

jsquyres commented May 2, 2024

I hit another issue with this libevent release though. It does not work with newer rpmbuild due to ambiguous python shebang because of this line. I had to manually change it to #!/usr/bin/env python3

You should note this in the commit message.

rpmbuild now does not accept '-' in release name.
This patch rebuilds libevent 2.1.12-stable with:
- autoconf 2.27
- automake 1.16.5
- libtool  2.4.7

Note that this libevent release contains ambiguous python shebang that will
cause rpm build failures on modern systems. Read more:
https://fedoraproject.org/wiki/Changes/Make_ambiguous_python_shebangs_error

Signed-off-by: Wenduo Wang <wenduwan@amazon.com>
@wenduwan
Copy link
Contributor Author

wenduwan commented May 6, 2024

Reworded commit

Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

With your new commit wording, I'm now confused.

Your new commit says:

Note that this libevent release contains ambiguous python shebang that will
cause rpm build failures on modern systems. Read more:
https://fedoraproject.org/wiki/Changes/Make_ambiguous_python_shebangs_error

which implies that the new tarball does not fix the issue. But your previous comment says:

I had to manually change it to #!/usr/bin/env python3

Which I interpreted to mean that you had manually changed it in the new tarball.

Which is it?

@wenduwan
Copy link
Contributor Author

wenduwan commented May 7, 2024

There are 2 issues exposed by newer rpmbuild in the current libevent release tarball.

  1. The file name libevent-2.1.12-stable-ompi is invalid. The release string stable-ompi cannot contain -. This is totally on us to fix. This PR addresses that.
  2. The upstream libevent contains ambiguous python shebang. This must be fixed upstream in a future release. We cannot fix it on ompi.

Given the slack discussion I'm inclined to cancel this PR and leave it broken - it actually does what we want, i.e. fending off people trying to build rpm with the internal libevent. Personally I want to fix it and allow people to build RPMs in whichever way possible, since we don't provide any warranty on the software anyways.

@wenduwan wenduwan closed this May 7, 2024
@jsquyres
Copy link
Member

jsquyres commented May 7, 2024

Why can't we fix the confusing shebang in the internal libevent tarball?

You already modified the tarball by running new autotools on it. What's wrong with making another small, patch-level change to that tarball?

@wenduwan
Copy link
Contributor Author

wenduwan commented May 7, 2024

IMO modifying the distributed source code is different - to begin with, I'm actually not sure which python version should be used.

@jsquyres
Copy link
Member

jsquyres commented May 7, 2024

It's the moral equivalent of distributing a patch -- which is what just about every binary packager does. Indeed, in OMPI, we patch a bunch of stuff before using it.

You should use python3.

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.

None yet

2 participants