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

loading: work on simplifications (and some corrections) of docs #29946

Merged
merged 8 commits into from
Dec 17, 2018

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 6, 2018

Split off doc changes from #29807, to try to get review (or approval to proceed) from Stefan.

@StefanKarpinski
Copy link
Member

Thanks for splitting this out—much easier to review this way!

@vtjnash
Copy link
Member Author

vtjnash commented Nov 7, 2018

Np, I've been meaning to do this for awhile. I just had originally figured that I would start with working on them together.

@vtjnash
Copy link
Member Author

vtjnash commented Nov 30, 2018

bump.

@vtjnash
Copy link
Member Author

vtjnash commented Dec 3, 2018

I'll merge in a few days if no further comments.

@StefanKarpinski
Copy link
Member

I will also review this week. If I haven't gotten to it by triage, then you can merge.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

There are definitely some good clarifications and improvements here. The middle bit that changes the description of how path maps work seems fairly wrong so we should probably drop that part. I've made a number of edit suggestions in places where I felt that the edits were incorrect or unclear.

base/initdefs.jl Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
### Package directories

Package directories provide a kind of environment that approximates package loading in Julia 0.6 and earlier, and which resembles package loading in many other dynamic languages. The set of packages available in a package directory corresponds to the set of subdirectories it contains that look like packages: if `X/src/X.jl` is a file in a package directory, then `X` is considered to be a package and `X/src/X.jl` is the file Julia loads to get `X`. Which packages can "see" each other as dependencies depends on whether they contain project files, and if they do, on what appears in those project files' `[deps]` sections.
Package directories provide a simpler environment, but less powerful or controlled, and they exist in two flavors (depending on whether they contain a project file). The set of root packages available is defined simply as the set of files in the directory that "look like" packages. A package `Name` is determined to exist if it finds one of the following files:
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct: if there's project file then it's always interpreted as a project environment, never as a package directory—that's the litmus test for which one you have.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Package directories provide a simpler environment, but less powerful or controlled, and they exist in two flavors (depending on whether they contain a project file). The set of root packages available is defined simply as the set of files in the directory that "look like" packages. A package `Name` is determined to exist if it finds one of the following files:
Package directories provide a simpler kind of environment with less control and without the ability to handle name collisions. In a package directory, the set of root packages available is just determined by the subdirectories that "look like" packages. A package `X` is determined to exist in a package directory if it contains one of the following "entry point" files:

- A file `Name.jl`
- A file `Name/src/Name.jl`
- A file `Name.jl/src/Name.jl`
Additionally, which packages it is able to import as dependencies depends on whether it contains a project file. If it does not, it can import any root package. Otherwise, if it does contain a `Package.toml` file, it can only import those packages which are identified in its `[deps]` sections (by uuid and name).
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you were thinking above of what dependencies the packages in a package directory can load? If there's a project file in a package in a package directory, its deps section determines what it can load and what each dependency name means. Otherwise, package names are looked up at the top level like in the REPL or Main.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I wanted to motivate the later sections, but the first part of this change doesn't read correctly.

doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
@StefanKarpinski StefanKarpinski self-assigned this Dec 7, 2018
@StefanKarpinski
Copy link
Member

I've applied my concrete editing suggestions. I still think switching from X to Name as a placeholder for a package name is confusing and that X is a much more obvious placeholder name. The changes in the middle are not resolved and seem to confuse project environments, package directories and stacked environments. A project environment must have a project file and is a more basic notion than a stacked environment: the load path is in no way involved in any of the behaviors of project environments or package directories. The load path search mechanism only comes in when you combine project environments and package directories by stacking them.

Copy link
Member Author

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Yep, I like most of those suggestions, and have attempted to re-work a few of the remaining areas to be clearer. If GitHub isn't showing a simple diff, looking at the commits on the command line may be nicer: git show --word-diff=color HEAD^

doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Some proposed edits.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Some requested changes.

@StefanKarpinski
Copy link
Member

I've reviewed this again and I think the changes are generally correct now, so you can merge when you're happy. You may want to take another pass through and edit it further.

@@ -179,7 +179,7 @@ If, on the other hand, Julia was loading the *other* `Priv` package—the one wi
1. `/home/me/.julia/packages/Priv/HDkrT`
2. `/usr/local/julia/packages/Priv/HDkrT`

Julia uses the first of these that exists to try to load the public `Priv` package, either from the file `HDkrT`, or the file `HDKrT/src/Priv.jl`.
Julia uses the first of these that exists to try to load the public `Priv` package from the file `packages/Priv/HDKrT/src/Priv.jl` in the depot where it was found.
Copy link
Member Author

Choose a reason for hiding this comment

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

OR the file packages/Priv/HDKrT—or is the intent to correct the code to match?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that should be corrected, but that can be done separately. In the meantime, we should not document something that we don't want people to use. Since these directories are not generally setup manually, I'm quite certain that we can still change this as a "minor change".

### Package directories

Package directories provide a kind of environment that approximates package loading in Julia 0.6 and earlier, and which resembles package loading in many other dynamic languages. The set of packages available in a package directory corresponds to the set of subdirectories it contains that look like packages: if `X/src/X.jl` is a file in a package directory, then `X` is considered to be a package and `X/src/X.jl` is the file Julia loads to get `X`. Which packages can "see" each other as dependencies depends on whether they contain project files, and if they do, on what appears in those project files' `[deps]` sections.
Package directories provide a simpler kind of environment without the ability to handle name collisions. In a package directory, the set of top-level packages is the set of subdirectories that "look like" packages. A package `X` is exists in a package directory if the directory contains one of the following "entry point" files:
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
Package directories provide a simpler kind of environment without the ability to handle name collisions. In a package directory, the set of top-level packages is the set of subdirectories that "look like" packages. A package `X` is exists in a package directory if the directory contains one of the following "entry point" files:
Package directories provide a simpler kind of environment with more cumbersome handling of name collisions and less careful reproducibility of versions. In a package directory, the set of top-level packages is the set of subdirectories that "look like" packages. A package `X` is exists in a package directory if the directory contains one of the following "entry point" files:

Copy link
Member

@StefanKarpinski StefanKarpinski Dec 12, 2018

Choose a reason for hiding this comment

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

If you're just in a package directory environment, you cannot handle package name collisions at all. In a stacked environment where some of the things in the stack are package directories name collisions are fine but that's a property of the stack, not the package directory so I don't think that's relevant to this description.

I'm guessing that "less careful reproducibility of versions" is referring to the fact that one can't snapshot the exact set of versions in a project file, but I feel like that's incidental: if you check the full contents of a package directory into git and version control it, then it's equally reproducible, it's just less simply to snapshot.

So I stand by my wording of this.

@vtjnash
Copy link
Member Author

vtjnash commented Dec 12, 2018

Great. I looked though and only had one final change to suggest, if you approve.

@fredrikekre fredrikekre added docs This change adds or pertains to documentation packages Package management and loading labels Dec 12, 2018
@vtjnash vtjnash merged commit 8b35e84 into master Dec 17, 2018
@vtjnash vtjnash deleted the jn/loading-refactoring-docs branch December 17, 2018 21:22
KristofferC pushed a commit that referenced this pull request Dec 20, 2018
@KristofferC KristofferC mentioned this pull request Dec 20, 2018
52 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants