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

sync: Use options from the txt file #464

Closed
wants to merge 2 commits into from

Conversation

suutari-ai
Copy link
Contributor

The requirements.txt file might have index or find-links options which
affects finding of the installable packages. Parse those options from
the txt file before doing the sync.

Utilize the pip options parsing code from the compile command which is
now moved to a separate file.

suutari-ai added a commit to suutari/prequ that referenced this pull request Mar 5, 2017
Merge PR jazzband#464.

* 'sync-use-opts-from-txt' of github.com:suutari-ai/prequ:
  sync: Use options from the txt file
suutari-ai added a commit to suutari/prequ that referenced this pull request Mar 5, 2017
Add entry about merged PR jazzband#464.

Fixes #1
@vphilippon
Copy link
Member

(Catching up)
The feature looks good to me.
This would need a rebase on master and some tests too, and that should be good.

@vphilippon vphilippon self-requested a review November 23, 2017 22:58
@suutari-ai suutari-ai force-pushed the sync-use-opts-from-txt branch 2 times, most recently from 8505e95 to 294b5fb Compare January 2, 2018 15:43
suutari-ai and others added 2 commits January 7, 2018 11:37
The requirements.txt file might have index or find-links options which
affects finding of the installable packages.  Parse those options from
the txt file before doing the sync.

Utilize the pip options parsing code from the compile command which is
now moved to a separate file.
Make sure that sync command passes the options specified in the txt file
to the issued "pip install" command.
@suutari
Copy link
Contributor

suutari commented Jan 7, 2018

Rebased and added some tests too.

install_flags.extend(['-f', link])
if no_index:
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change the no_index flag is inhibited. Is this wanted?

Copy link
Contributor

Choose a reason for hiding this comment

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

The no_index flag is passed to the get_pip_options_and_pypi_repository call which then passes it along to the finder in the constructed repository. That'll make the repository.finder.index_urls to be empty if no_index is True. It'll also consider --no-index specified in the parsed txt file.

if extra_index_url:
for extra_index in extra_index_url:
install_flags.extend(['--extra-index-url', extra_index])
for (i, index_url) in enumerate(repository.finder.index_urls):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not quite the same intent. I see that you are reconstructing the parameter invocation based upon the index_urls, but that exposes too much of the implementation detail. I don't quite grasp why you are doing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

The point here is that the call to pip.req.parse_requirements above (line 56) will update the flags in the repository.finder (which is passed to it) according to the flags specified in the txt file. Therefore the repository.finder.index_urls will contain combination of command line options and txt file options and this is then used here to construct the options given to pip install.

Copy link
Contributor

@suutari suutari Jan 7, 2018

Choose a reason for hiding this comment

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

I don't understand what you mean by the "implementation detail", since there wasn't really any separation between implementation and interface here before my changes either. If implementation details should be hidden, then there should be a class like RequirementsFileParser which then could be used to parse the txt files and as a result would output the list of requirements and pip options from the parsed files. That would be nice improvement, but completely another issue.

Can you elaborate what do you mean by exposing the implementation detail here?

Copy link

@RDIL RDIL left a comment

Choose a reason for hiding this comment

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

Please fix conflicts.

@atugushev atugushev added enhancement Improvements to functionality needs rebase Need to rebase or merge labels Sep 26, 2019
@atugushev
Copy link
Member

Superseded by #824.

@atugushev atugushev closed this Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to functionality needs rebase Need to rebase or merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants