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

Add pipopts on install-from-bindep #468

Merged
merged 1 commit into from
Apr 25, 2023
Merged

Conversation

tanganellilore
Copy link
Contributor

Team,

this pull request add support to PIP_OPTS on install-from-bindep, as same of assemble file.

So with this implementation we can add it and allow us to use PIP_OPTS option with new v3 format.

@tanganellilore tanganellilore requested a review from a team as a code owner April 6, 2023 10:35
@Akasurde Akasurde changed the title Add pipotps on install-from-bindep Add pipopts on install-from-bindep Apr 7, 2023
@tanganellilore
Copy link
Contributor Author

@Shrews this is reasonable?

@Shrews
Copy link
Contributor

Shrews commented Apr 11, 2023

@Shrews this is reasonable?

It does seem reasonable. The thing we need to put some thought into is tests. We only just incorporated these scripts into this repo from a separate repo (they would normally be expected to exist in the base image, but we get rid of that requirement in this release). Any changes to these scripts, just like the rest of the code, must include a test to validate the change. Unfortunately for you, this is the first PR to one of these scripts, and we do not yet have a testing strategy in place. If you have a good idea for testing this change, I encourage you to add it to the PR and let us evaluate it. Otherwise, the team will need to find time to put our heads together on a testing strategy.

@Shrews
Copy link
Contributor

Shrews commented Apr 11, 2023

@tanganellilore Is this change even really necessary? I understand wanting to use PIP_OPTS in assemble, but builder's use of install-from-bindep script is really just to install the wheel packages that were built by assemble. Do we actually gain anything by adding PIP_OPTS support here?

@tanganellilore
Copy link
Contributor Author

Hi team,
sorry for delay but i was focused on other stuff and other issue on awx project.
What your ad saying @Shrews should be correct, in the assemble we should have all packages that is requested in final step, but this seems not happen in my test env build.

As you can see on this pastebin, after some library that is fetched from "cache" (the output dir of assemble), some new library are requested and pip try to loop on each library to find the correct one.

I try to find why this happen, but anyway, adding an option parameter, not brake anything and increase the possibility to customize pip installation.

Thanks

@tanganellilore
Copy link
Contributor Author

Hi team,
one more point.
After some discussion with @shanemcd on matrix, I follow him suggestion to upgrade pip in base imge, and seems to work without pipopts in the final stage.

Record log is here: pastebin

I'm still using PIP_OPTS=--use-deprecated=legacy-resolver on other layers

@Shrews Shrews merged commit 9df085a into ansible:devel Apr 25, 2023
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.

3 participants