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

julia: Only run Pkg.clone(pwd()) on < 0.7- #1392

Merged
merged 6 commits into from
Jun 4, 2018

Conversation

staticfloat
Copy link
Contributor

@staticfloat staticfloat commented May 28, 2018

Recent changes to Julia 0.7 have obviated the need for a Pkg.clone() step; so if there is a Project.toml file in the current directory, and we're running on Julia 0.7+, then don't bother to Pkg.clone(pwd()). If there is no Project.toml file, or if we're running on Julia 0.6, then we should still Pkg.clone(pwd()).

@KristofferC @ninjin @tkelman @simonbyrne

Recent changes to Julia 0.7 have obviated the need for a `Pkg.clone()` step; so if there is a `Project.toml` file in the current directory, and we're running on Julia 0.7+, then don't bother to `Pkg.clone(pwd())`.  If there is no `Project.toml` file, or if we're running on Julia 0.6, then we should still `Pkg.clone(pwd())`.
@tkelman
Copy link
Contributor

tkelman commented May 28, 2018

this only clones if project.toml exists, and the version cutoff should be specific. what happens now when project.toml exists that makes this unnecessary?

@staticfloat
Copy link
Contributor Author

the version cutoff should be specific

I figured this wasn't a problem, as it's impossible to get a 0.7- version that doesn't have the new Pkg code.

what happens now when project.toml exists that makes this unnecessary?

Pkg3 is much more intelligent about dealing with packages that are outside of the ~/.julia directory; so it's no longer necessary (and indeed, it doesn't work anymore) to clone a package from the current directory, because Pkg.clone() now implicitly clones into the current directory. Nowadays, you can just Pkg.test() and it will load the packages as described within the Project.toml file into LOAD_PATH; which means we no longer have to have packages sitting in ~/.julia/v0.X for them to be found by Pkg.test().

@tkelman
Copy link
Contributor

tkelman commented May 28, 2018

Old nightlies will be > v"0.7-" and not have the new code.

So Pkg operations depend on where you run them from? Creating a project.toml in the current directory can change the behavior to looking locally? Strange. And doesn't Pkg3 look for either JuliaProject.toml or Project.toml?

@staticfloat
Copy link
Contributor Author

staticfloat commented May 28, 2018

Old nightlies will be > v"0.7-" and not have the new code.

Right, but you can't get old nightlies on Travis, can you? Can you explicitly download a particular git hash of julia? If someone does do that, I'm comfortable with telling them they should expect things to be broken.

So Pkg operations depend on where you run them from?

Yes, in a similar vein to how git operations depend on where you run them from.

And doesn't Pkg3 look for either JuliaProject.toml or Project.toml?

You are correct; I should check for both here.

@tkelman
Copy link
Contributor

tkelman commented May 29, 2018

The file existence check should be combined in the julia code - you don't want to suppress cloning on 0.6 if that file exists

@tkelman
Copy link
Contributor

tkelman commented May 29, 2018

No, still not right. On 0.6, and early 0.7-DEV, you want to clone no matter what. Only on very recent 0.7-DEV does the existence of the project files matter.

@staticfloat
Copy link
Contributor Author

and early 0.7-DEV

You can't install early 0.7-DEV on Travis. All nightlies are now past that point; and the 0.7 alpha has been tagged. Just as we don't test for feature differentiations within 0.6 on Travis, I don't think it makes sense to test for feature differentiations within 0.7 now that the binaries are beyond that point. If this were within a package, I would agree with you (as who knows what versions people are running locally), or if there were a way for users to ask Travis for a particular version of Julia, I would agree that however unlikely, it's worth putting in that exact version check, but as it stands, I think that it's better to not pull apart version numbers quite so harshly; especially for future readers of the code.

@tkelman
Copy link
Contributor

tkelman commented May 29, 2018

This still shouldn't be behaving any differently depending on the existence of project files on 0.6. And it's 3-4 characters difference to get it correct. This code will show up in logs and get copy-pasted who knows where, and the feature of being able to ask for specific sha's could fairly easily be implemented here. There's no good reason not to be more specific.

@ghost
Copy link

ghost commented May 29, 2018

Thank you @staticfloat for handling this, I should have thought about the need for the update myself. I think @tkelman is right though, it would be better to code as defensively as is humanly positive in this case given the above reasons. If I had the cycles, that specific SHA feature would certainly be something useful in the future. Especially pinning a test just before some major break in syntax, it may actually encourage package developers to stay more up to date with master, rather than abandoning it once the effort is too high to keep up – as it is now.

@KristofferC
Copy link

In my opinion, trying to keep things compatible commit for commit on nightly is a pointless endeavour and adds maintenance cost and complications. In 0.7, Pkg3 is the new package manager and should use Pkg.test() for new packages. This seems to achieve exactly that.

@staticfloat
Copy link
Contributor Author

Okay, with this latest push, do we have quorum?

@tkelman
Copy link
Contributor

tkelman commented May 31, 2018

Mostly the wrong repo to discuss this, but the feature this is relying on is extremely questionable - it's not like git behaving differently depending which directory you run it from (git operations are generally implicitly w.r.t. the repo that the current directory is part of, unless you set GIT_DIR), it's a lot more like if virtualenv were to automatically activate a new environment any time you happen to be in a directory that contains a file named requirements.txt. The current directory isn't usually treated by package managers like pip or apt as the piece of state most relevant to where packages should be pulled from and how to map names to code locations.

@KristofferC
Copy link

Mostly the wrong repo to discuss this

Strongly agree.

@staticfloat
Copy link
Contributor Author

@BanzaiMan Looks like this is ready for your perusal when you are. :)

@BanzaiMan BanzaiMan merged commit d9bb10b into travis-ci:master Jun 4, 2018
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.

4 participants