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

Use pbr rather than direct easy install #1312

Closed
wants to merge 3 commits into from

Conversation

jokke-ilujo
Copy link

Adds 'pbr' as dependency.

Adds '.venv' and 'build/' to gitignore.
Introduces wgsi entrypoint gnocchi-app.
Removes direct calls to setuptools easy install in favour of letting pbr to handle the install.

Closes Issue: 1304

@jokke-ilujo jokke-ilujo marked this pull request as draft July 28, 2023 13:02
@jokke-ilujo
Copy link
Author

Looks like we need to pin the setuptools <68.0.0 on stable/4.4 first to unblock the upgrade job. After that this PR should do the trick for path forward.

@jokke-ilujo jokke-ilujo reopened this Jul 31, 2023
@jokke-ilujo jokke-ilujo marked this pull request as ready for review July 31, 2023 07:18
@tobias-urdin
Copy link
Contributor

I don't have an strong opinion on this, pbr was removed in the past [1]. What would be needed here is clearing with the distribution packagers to see what they prefer.

Since Gnocchi is still heavily integrated into the OpenStack ecosystem I suspect it would not be an issue but I wouldn't want to break their world either.

If anybody is faster to clear up that detail we could just merge this and have it land in next release.

[1] 3f8a22a

@jokke-ilujo
Copy link
Author

I don't have an strong opinion on this, pbr was removed in the past [1]. What would be needed here is clearing with the distribution packagers to see what they prefer.

Since Gnocchi is still heavily integrated into the OpenStack ecosystem I suspect it would not be an issue but I wouldn't want to break their world either.

If anybody is faster to clear up that detail we could just merge this and have it land in next release.

[1] 3f8a22a

That explains the references to pbr but no evidence of it being in use. :D

Totally understand the concern for packagers quality of life. I'd like to argue at least pbr is fairly actively maintained unlike evidently the special sauce why it was dropped in the first place. Like you mentioned due to the close relationship pbr is also well understood from the OpenStack side and could hopefully ease the pain next time something breaks.

Let me know if you want me to resolve those merge conflicts and address any other possible concerns and I'll try to look after this.

@tobias-urdin
Copy link
Contributor

I don't have an strong opinion on this, pbr was removed in the past [1]. What would be needed here is clearing with the distribution packagers to see what they prefer.
Since Gnocchi is still heavily integrated into the OpenStack ecosystem I suspect it would not be an issue but I wouldn't want to break their world either.
If anybody is faster to clear up that detail we could just merge this and have it land in next release.
[1] 3f8a22a

That explains the references to pbr but no evidence of it being in use. :D

Totally understand the concern for packagers quality of life. I'd like to argue at least pbr is fairly actively maintained unlike evidently the special sauce why it was dropped in the first place. Like you mentioned due to the close relationship pbr is also well understood from the OpenStack side and could hopefully ease the pain next time something breaks.

Let me know if you want me to resolve those merge conflicts and address any other possible concerns and I'll try to look after this.

I wholeheartedly agree with you, perhaps we can get some feedback from @amoralej and @javacruft here.

@javacruft
Copy link
Contributor

@tobias-urdin as stated PBR is pretty well understood and widely used across the OpenStack ecosystem so no issue from Ubuntu packaging team perspective.

@amoralej
Copy link

amoralej commented Aug 1, 2023

+1 to move back to pbr. I think the idea of moving out of was related to making gnocchi less openstack-centric but IMO pbr works well, is well maintained and widely used. With my RDO packager hat, I prefer gnocchi to be consistent with the OpenStack ecosystem (FTR, moving to setuptools_scm was a problem for us in the past).

@tobias-urdin
Copy link
Contributor

@jokke-ilujo I think we have atleast an majority consensus here, I've also contacted @thomasgoirand for Debian packaging feedback.

I have a question thought, can we keep the wsgi_script as gnocchi-api to prevent changing any paths, it should be possible for that to coexist with the gnocchi-api command right without us having to change to gnocchi-app like this change does?

Also to be explicitly clear for anybody reading this, we would not be backporting the addition of PBR.

@amoralej
Copy link

amoralej commented Aug 1, 2023

I have a question thought, can we keep the wsgi_script as gnocchi-api to prevent changing any paths, it should be possible for that to coexist with the gnocchi-api command right without us having to change to gnocchi-app like this change does?

Yes, please, we should maintain backwards-compatibility, at least for deprecation notice and adapt tooling (or simply leave it as-is).

@jokke-ilujo
Copy link
Author

I'll try to figure it out. It isn't at least obvious without digging ourselves back into the very same easy-install hole we're trying to climb out of. If you define same entry-point as console_script and wsgi_script for setuptools, only the console_script gets generated.

@tobias-urdin tobias-urdin mentioned this pull request Aug 2, 2023
@stephenfin
Copy link
Contributor

I don't think this is a good move and I struggle to see what problem it resolves.

I'll try to figure it out. It isn't at least obvious without digging ourselves back into the very same easy-install hole we're trying to climb out of. If you define same entry-point as console_script and wsgi_script for setuptools, only the console_script gets generated.

This is my main concern. Ultimately pbr is relying on deprecated APIs that are likely to be removed at some point in the future. From what I can tell, it seems the console_scripts stuff is handled by pip nowadays rather than setuptools. If we want to support wsgi_scripts in the new world, we're going to have to talk to the Python Packaging Authority (PyPA) folks (Donald Stufft et al.). Absent that, I suspect we will need to resort to the legacy script stuff and all the problems that entails 😞 Using pbr doesn't magically fix anything though.

@stephenfin
Copy link
Contributor

Let's see what appetite is there for this https://discuss.python.org/t/adding-support-for-wsgi-scripts-entrypoint/30905

Adds 'pbr' as dependency.

Adds '.venv' and 'build/' to gitignore.
Removes direct calls to setuptools easy install in
favour of letting pbr to handle the install.

Closes Issue: 1304
@jokke-ilujo
Copy link
Author

Hmm-m the docs failures really aren't that clear. Any ideas?

@tobias-urdin
Copy link
Contributor

After the feedback from @stephenfin and the patch #1327 (and #1325 though unrelated to PBR) I tend to lean more to that then to migrate to PBR.

Waiting for feedback from all of you before moving forward with that, thanks!

@mrunge
Copy link
Contributor

mrunge commented Aug 11, 2023

I really like @stephenfin patches

@paramite
Copy link

+1

@mrunge
Copy link
Contributor

mrunge commented Aug 17, 2023

@tobias-urdin what would be your preference to proceed here? This unfortunately blocks OpenStack Telemetry upstream gates in all branches

@tobias-urdin
Copy link
Contributor

@mrunge do you mean supporting newer setuptools? that should be fixed by this b52f741 that is also backported to stable branches just not released yet.

@mrunge
Copy link
Contributor

mrunge commented Aug 17, 2023

oops, I certainly missed b52f741 .
I just stumbled upon the failures in telemetry ci again.

@tobias-urdin
Copy link
Contributor

Closing in favor of merged #1327

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

Successfully merging this pull request may close these issues.

None yet

7 participants