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.

Sign up for GitHub

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

Jump to bottom

Fix download agent search relying on throwing of Sys.which(). Update t #28157 #28682

Merged
merged 6 commits into from
Sep 4, 2018
Merged

Fix download agent search relying on throwing of Sys.which(). Update t #28157 #28682

merged 6 commits into from
Sep 4, 2018

Conversation

wfrgra
Copy link
Contributor

@wfrgra wfrgra commented Aug 16, 2018 •
edited by stevengj
Loading

Closes #28157.

stevengj
stevengj reviewed Aug 16, 2018
View reviewed changes
base/download.jl Outdated
end
end
if downloadcmd == "wget"
if !isempty(Sys.which("wget"))
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be Sys.which("wget") !== nothing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, not sure where I got the idea that empty strings were being returned

@stevengj stevengj added the bugfix This change fixes an existing bug label Aug 17, 2018
@staticfloat
Copy link
Member

Note that this changes behavior slightly, in that it will search the PATH every single time we invoke it, rather than caching the result as was done previously. I personally don't care that much, as I think that's kind of a premature optimization, but it was there.

@StefanKarpinski
Copy link
Member

Seems like a bug fix to me where the bug is "Sys.which does not pick up changes to PATH".

@stevengj
Copy link
Member

Causing a CI failure:

Error in testset file:
Test Failed at /tmp/julia/share/julia/test/file.jl:878
  Expression: download("ba\0d", "good")
    Expected: ArgumentError
      Thrown: Base.IOError
ERROR: LoadError: Test run finished with errors
in expression starting at /tmp/julia/share/julia/test/runtests.jl:61

@KristofferC KristofferC mentioned this pull request Aug 20, 2018
Merged
@KristofferC
Copy link
Member

Can this be tested?

stevengj
stevengj reviewed Aug 20, 2018
View reviewed changes
base/download.jl Outdated
@@ -29,7 +29,7 @@ else
try
run(`wget -O $filename $url`)
catch
rm(filename) # wget always creates a file
isfile(filename) && rm(filename) # wget always creates a file
Copy link
Member

Choose a reason for hiding this comment

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

just do rm(filename, force=true)

@KristofferC
Copy link
Member

Bump, what should we do here? Just merge?

@KristofferC KristofferC merged commit 14fb104 into JuliaLang:master Sep 4, 2018
KristofferC pushed a commit that referenced this pull request Sep 4, 2018
@wfrgra @KristofferC
Fix download agent search relying on throwing of Sys.which(). Update t …
e82109b 
…#28157 (#28682)

* Fix download agent search relying on throwing of Sys.which()

(cherry picked from commit 14fb104)
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
@wfrgra @KristofferC
Fix download agent search relying on throwing of Sys.which(). Update t …
51b187f 
…#28157 (#28682)

* Fix download agent search relying on throwing of Sys.which()

(cherry picked from commit 14fb104)
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
@wfrgra @KristofferC
Fix download agent search relying on throwing of Sys.which(). Update t …
d6e43e2 
…#28157 (#28682)

* Fix download agent search relying on throwing of Sys.which()

(cherry picked from commit 14fb104)
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
@wfrgra @KristofferC
Fix download agent search relying on throwing of Sys.which(). Update t …
47d4cce 
…#28157 (#28682)

* Fix download agent search relying on throwing of Sys.which()

(cherry picked from commit 14fb104)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevengj stevengj stevengj approved these changes

@staticfloat staticfloat Awaiting requested review from staticfloat

Assignees
No one assigned
Labels
bugfix This change fixes an existing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wfrgra @staticfloat @StefanKarpinski @stevengj @KristofferC @WillRam