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

check if a download is successfull before deciding not to try the nex… #609

Merged
merged 6 commits into from Jul 25, 2016

Conversation

Projects
None yet
3 participants
@JensTimmerman
Copy link
Contributor

JensTimmerman commented Jun 8, 2016

…t possible dist


if local_index is not None:
dist = dist or find(requirement, local_index)
dist, mylocation = dist, mylocation if dist else find(requirement, local_index)

This comment has been minimized.

@jayvdb

jayvdb Jun 8, 2016

Contributor

Two consecutive spaces here


if force_scan:
self.prescan()
self.find_packages(requirement)
dist = find(requirement)
dist, mylocation = find(requirement)

This comment has been minimized.

@jayvdb

jayvdb Jun 8, 2016

Contributor

Since you are modifying this chunk...maybe?

if not dist and local_index is not None:

This comment has been minimized.

@JensTimmerman

JensTimmerman Jun 8, 2016

Author Contributor

indeed, that would be nicer, fixed.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jun 24, 2016

Can you supply a justification for this change? What behavior led you to make the change? How does the change improve the behavior? Are there any backward compatible considerations?

mylocation = self.download(dist.location, tmpdir)
if os.path.exists(mylocation):
return dist, mylocation
return None, None

This comment has been minimized.

@jaraco

jaraco Jun 24, 2016

Member

The presence of None, None is a bad smell to me. Better would be to return above a single value which wraps the necessary components, maybe a namedtuple('DistLocation', 'dist location').

This comment has been minimized.

@jaraco

jaraco Jun 24, 2016

Member

Or maybe augment the resulting dist with a ._found_location attribute or similar.

This comment has been minimized.

@JensTimmerman

JensTimmerman Jun 24, 2016

Author Contributor

sure, I can clean this up,
thought this would be ok, since process_index inner scan(link) function already does the same ;-)

This comment has been minimized.

@jaraco

jaraco Jun 24, 2016

Member

No worries. I acknowledge - there are some old and undesirable patterns in setuptools, given its 11 year history.

@JensTimmerman

This comment has been minimized.

Copy link
Contributor Author

JensTimmerman commented Jun 24, 2016

the problem this fixes for me is: when I specify dependency_links as something like

 dependency_links=[
        "git+https://server1.example.com/mydependency.git@1.5.5#egg=mydependnecy-1.5.5",
        "git+ssh://server2.example.com/mydependency.git@1.5.5#egg=mydependnecy-1.5.5",
],

where server1 and server2 could e..g be a public github.com and a private github enterprise,
and public network is not available, then setuptools will currently fail to locate this dependency.
(changing the order of the links doesn't seem to matter)
However, with this change in place, after the first download fails, it tries the second download, and succeeds.

I could see more cases where it would be beneficial to try a second location if it exists, if the first download fails. Since, we have a second location, why not?

@JensTimmerman

This comment has been minimized.

Copy link
Contributor Author

JensTimmerman commented Jun 24, 2016

backwards compatibility: I don't believe a lot of work-flows would rely on the fact that an installation won't happen if the first tried link doesn't work, because this first tried link seems to be random. Depending on a few factors the first or second link will be tried first or last.

This does check if the download path exists, so if downloaders are downloading something that does not result in an existing path this could break their behaviour, but what is a download if it doesn't result in a valid path?

if local_index is not None:
dist = dist or find(requirement, local_index)
if not dist and local_index is not None:
dist = find(requirement, local_index)

This comment has been minimized.

@JensTimmerman

JensTimmerman Jun 24, 2016

Author Contributor

this change is not relevant to this pr, but this happens to be a nicer way to check, so it's in here.

@JensTimmerman

This comment has been minimized.

Copy link
Contributor Author

JensTimmerman commented Jun 24, 2016

old behaviour:

Searching for vsc-base>=2.4.16
Best match: vsc-base 2.4.16
Doing git clone from ssh://github.ugent.be/hpcugent/vsc-base.git to /tmp/easy_install-qVryB5/vsc-base.git
ERROR: Repository not found.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
Processing vsc-base.git
error: Couldn't find a setup script in /tmp/easy_install-qVryB5/vsc-base.git

new behaviour:

Searching for vsc-base>=2.4.16
Doing git clone from ssh://github.ugent.be/hpcugent/vsc-base.git to /tmp/easy_install-A9wgLC/vsc-base.git
ERROR: Repository not found.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
Doing git clone from https://github.com/hpcugent/vsc-base.git to /tmp/easy_install-A9wgLC/vsc-base.git
Best match: vsc-base 2.4.16
Processing vsc-base.git
Writing /tmp/easy_install-A9wgLC/vsc-base.git/setup.cfg
Running setup.py -q bdist_egg --dist-dir /tmp/easy_install-A9wgLC/vsc-base.git/egg-dist-tmp-djmMaJ
...
@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jul 24, 2016

Can you please add an entry to the changelog (0.1 bump) to describe to the users what they should expect from the change?

@JensTimmerman JensTimmerman force-pushed the JensTimmerman:check_downloads branch from 72673f6 to 3856953 Jul 25, 2016

@JensTimmerman JensTimmerman force-pushed the JensTimmerman:check_downloads branch from 3856953 to 87c7762 Jul 25, 2016

@JensTimmerman

This comment has been minimized.

Copy link
Contributor Author

JensTimmerman commented Jul 25, 2016

@jaraco added changes and rebased on top of master

@jaraco jaraco merged commit 1b94bba into pypa:master Jul 25, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@JensTimmerman JensTimmerman deleted the JensTimmerman:check_downloads branch Jul 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.