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

Remove wrapper script #11

Merged
merged 12 commits into from
Sep 13, 2017
Merged

Conversation

dfornika
Copy link
Contributor

Addresses #10.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jakirkham
Copy link
Member

So I think this content needs to be moved to juliarc.jl. Should be in $PREFIX/etc/julia/juliarc.jl.

@jakirkham
Copy link
Member

Tried to add this code for the juliarc instead of the wrapper script and it seems to work ok.

# $PREFIX/etc/julia/juliarc.jl

JULIA_PREFIX = abspath(joinpath(Base.source_path(), "..", "..", ".."))

if !("JULIA_PKGDIR" in keys(ENV))
    ENV["JULIA_PKGDIR"] = joinpath(JULIA_PREFIX, "share", "julia", "site")
    Pkg.init()
    Pkg.__init__()
    pop!(Base.LOAD_CACHE_PATH)
end

if !("JULIA_HISTORY" in keys(ENV))
    ENV["JULIA_HISTORY"] = joinpath(JULIA_PREFIX, ".julia_history")
end

@jakirkham
Copy link
Member

Added PR ( dfornika#1 ) against your branch with this sort of change.

* Add a custom juliarc.jl

Needed to ensure packages and other relevant information to the Julia
install are tracked in the conda environment.

* Install custom juliarc

Make sure that that our custom juliarc is available in the standard
location.

* Create the package directory

Make sure that Julia initializes our package directory and includes the
standard packages in it.

* Bump build number to 1

Needed to repackage after removal of the wrapper script, inclusion of
juliarc, and inclusion of the prepopulated package directory.

if !("JULIA_HISTORY" in keys(ENV))
ENV["JULIA_HISTORY"] = joinpath(JULIA_PREFIX, ".julia_history")
end
Copy link
Member

Choose a reason for hiding this comment

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

So I know I added this and it just imitates the wrapper script that we are replacing, but I don't think it is a good idea. If there is a multiuser conda install, we don't want to squash their history into one file shared by all of them. This is bad for user's tracking their own history and is bad if some of them lack permissions to the conda install. Instead we should keep them separated per user. Ideally this is done by allowing this to write in the user's home directory somewhere. This is actually what we do with IPython as well.

TL;DR We should remove these 3 lines.

Copy link
Member

Choose a reason for hiding this comment

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

that applies to users' packages too doesn't it?

Copy link
Member

Choose a reason for hiding this comment

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

That is a totally different view of package management from the one conda is based in. The point of using something like conda is to allow for multiple different independent environments where many different versions of packages and their dependencies live. This allows one to easily switch between different development and production environments trivially on a regular basis. Also users can trivially toss aside an environment or create a new one fresh without concerns of other dependencies leaking in. We can't very easily do any of these things if packages live outside the environment.

Copy link
Member

Choose a reason for hiding this comment

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

Should add that one can create conda environments anywhere. So if a user is working with a multiuser conda installation and does not have permissions to the multiuser environments (or wishes not to mess with them), they still can create an environment in their home directory or anywhere else with packages of their choosing.

Copy link
Member

Choose a reason for hiding this comment

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

won't that force them to reinstall julia? packages are user data, not part of the language, and shouldn't be forced to be installed in the same place

Copy link
Member

@jakirkham jakirkham Sep 13, 2017

Choose a reason for hiding this comment

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

It won't cache julia packages.

Sorry, what won't?

I don't think you should put user-installed julia packages under the language install tree.

Ok. Should we be using local or something to disambiguate? They are under site now, which doesn't overlap with base.

Copy link
Member

Choose a reason for hiding this comment

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

conda won't cache julia packages if they're placed as extra content inside a different installed package (the julia language)

most julia users would expect packages to be shared between different methods of installing the same minor version of julia. if conda starts managing julia packages (I'd advise against this until after the julia package manager has been rewritten) then you could add them to the LOAD_PATH without involving julia's package manager at all.

Copy link
Member

Choose a reason for hiding this comment

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

conda won't cache julia packages if they're placed as extra content inside a different installed package (the julia language)

Right, but the plan would be to package Julia packages as conda packages. At which point conda would be able to handle them like any other conda package.

most julia users would expect packages to be shared between different methods of installing the same minor version of julia.

This will be possible as we add conda packages of julia packages.

if conda starts managing julia packages (I'd advise against this until after the julia package manager has been rewritten) then you could add them to the LOAD_PATH without involving julia's package manager at all.

Ok noted.

I think this discussion has diverged quite a bit from the original PR, which is my fault. We should move this discussion about how to manage Julia packages within the context of conda into a new issue so that we can discuss this further. Would hate to have this discussion buried in outdated comment of an unrelated diff.

Copy link
Member

Choose a reason for hiding this comment

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

Both ways - a non-conda install of julia should be able to see packages added by running Pkg.add within a conda-installed julia. It might not see Julia packages added via conda install, but I don't think you should go there.

Copy link
Member

Choose a reason for hiding this comment

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

Have open issue ( #14 ) to discuss how best to manage Julia packages with conda. TBH this is something we are going to want to hammer out sooner rather than later. So we should keep an eye open for practical near term solutions.

recipe/build.sh Outdated

# Populate initial package directory
julia -e "Pkg.init(); Pkg.__init__()"
Copy link
Member

Choose a reason for hiding this comment

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

We may need to drop Pkg initialization step after all. Appears conda-build strips out .git directories, which will break the METADATA repo. There are some hacks we could use to workaround this, but I'm not sure they are worth it.

xref: conda/conda-build#2360

Copy link
Member

Choose a reason for hiding this comment

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

Added PR ( dfornika#2 ) to drop this. Should fix the build issue.

This does not work correctly as `conda-build` strips out the `.git`
directory from `METADATA`. There are no clean fixes for this issue. As
we already try to initialize the package directory when Julia starts,
this will be handled anyways on the user's machine. In the end, this may
be best to avoid having a potentially out-of-date index in the package.
@jakirkham
Copy link
Member

cc @tkelman

recipe/build.sh Outdated
chmod +x "$PREFIX/bin/julia"

# Configure juliarc to use conda environment
cp -f "${RECIPE_DIR}/juliarc.jl" "${PREFIX}/etc/julia/juliarc.jl"
Copy link
Member

Choose a reason for hiding this comment

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

should append, not replace

Copy link
Member

Choose a reason for hiding this comment

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

It looks like an empty file with a comment about how we can add stuff to it. Why not just replace it?

Copy link
Member

Choose a reason for hiding this comment

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

it isn't empty on all platforms

Copy link
Member

Choose a reason for hiding this comment

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

Ah. So what sort of stuff ends up there then?


if !("JULIA_PKGDIR" in keys(ENV))
ENV["JULIA_PKGDIR"] = joinpath(JULIA_PREFIX, "share", "julia", "site")
Pkg.init()
Copy link
Member

Choose a reason for hiding this comment

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

this probably shouldn't be done on every startup, only if it hasn't been initialized yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the preferred way to check if the package repository has been initialized?

Copy link
Member

Choose a reason for hiding this comment

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

Was thinking about that. It seems to only take a toll the first time (as it is initializing everything from scratch). What sort of penalties would we be taking on for subsequent calls?

Initially we tried to run this once during the build, but there was a conda-build issue that blocked it. ( conda/conda-build#2360 ) In the end, not sure we should be running this at build time anyways as it would go stale and make the package larger.

Could add a post-link step to handle this instead. That would run this step once only when julia gets installed. Seems like the best of both worlds really.

OTOH we could just never initialize Pkg at all. Would make it slow for the user the first time they run something with Pkg, but that would be the case anyways, right?

FWIW borrowed this strategy from SO answer. Related to this, is it necessary to call Pkg.__init__() or is that already handled by Pkg.init()?

recipe/build.sh Outdated
chmod +x "$PREFIX/bin/julia"

# Configure juliarc to use conda environment
"${PREFIX}/etc/julia/juliarc.jl" << "${RECIPE_DIR}/juliarc.jl"
Copy link
Member

Choose a reason for hiding this comment

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

I think you want cat "${RECIPE_DIR}/juliarc.jl" >> "${PREFIX}/etc/julia/juliarc.jl".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Was trying to avoid a 'useless use of cat award.'

if !("JULIA_PKGDIR" in keys(ENV))
ENV["JULIA_PKGDIR"] = joinpath(JULIA_PREFIX, "share", "julia", "site")
Pkg.init()
Pkg.__init__()
Copy link
Member

Choose a reason for hiding this comment

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

Let's go ahead and drop this and the line above it (i.e. Pkg.init()).


if !("JULIA_PKGDIR" in keys(ENV))
ENV["JULIA_PKGDIR"] = joinpath(JULIA_PREFIX, "share", "julia", "site")
pop!(Base.LOAD_CACHE_PATH)
Copy link
Member

@jakirkham jakirkham Sep 12, 2017

Choose a reason for hiding this comment

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

Need to change this line to Base.LOAD_CACHE_PATH[1] = joinpath(ENV["JULIA_PKGDIR"], "lib", string("v", join(split(string(VERSION), ".")[1:2], "."))) now that we have removed the Pkg.init() stuff.


if !("JULIA_PKGDIR" in keys(ENV))
ENV["JULIA_PKGDIR"] = joinpath(JULIA_PREFIX, "share", "julia", "site")
Base.LOAD_CACHE_PATH[1] = ENV["JULIA_PKGDIR"]
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I think my edits to that comment didn't show up.

This needs to be Base.LOAD_CACHE_PATH[1] = joinpath(ENV["JULIA_PKGDIR"], "lib", string("v", join(split(string(VERSION), ".")[1:2], "."))).

This makes it $PREFIX/share/julia/site/lib/v0.5 for 0.5.x.

@jakirkham
Copy link
Member

LGTM. This is consistent with its behavior from before and removes the wrapper script (the intended goal). While there may be other discussions/changes that need to occur, would recommend we save them for new issues/PRs.

@dfornika dfornika merged commit 19d96fa into conda-forge:v0.5 Sep 13, 2017
@jakirkham jakirkham mentioned this pull request Sep 13, 2017
@jakirkham
Copy link
Member

Thanks @dfornika. Also thanks @tkelman for the review.

jakirkham pushed a commit to jakirkham-feedstocks/julia-feedstock that referenced this pull request Sep 13, 2017
* Removed wrapper script

* Bumped build number

* Use juliarc (conda-forge#1)

* Add a custom juliarc.jl

Needed to ensure packages and other relevant information to the Julia
install are tracked in the conda environment.

* Install custom juliarc

Make sure that that our custom juliarc is available in the standard
location.

* Create the package directory

Make sure that Julia initializes our package directory and includes the
standard packages in it.

* Bump build number to 1

Needed to repackage after removal of the wrapper script, inclusion of
juliarc, and inclusion of the prepopulated package directory.

* Removed JULIA_HISTORY setting

* Drop package initialization (conda-forge#2)

This does not work correctly as `conda-build` strips out the `.git`
directory from `METADATA`. There are no clean fixes for this issue. As
we already try to initialize the package directory when Julia starts,
this will be handled anyways on the user's machine. In the end, this may
be best to avoid having a potentially out-of-date index in the package.

* Append juliarc.jl instead of replace

* Append juliarc.jl the right way

* Added post-link script for Pkg.init()

* Removed Pkg.init() from juliarc.jl

* Correct setting Base.LOAD_CACHE_PATH

* Correct setting Base.LOAD_CACHE_PATH again

(cherry picked from commit 19d96fa)
jakirkham pushed a commit to jakirkham-feedstocks/julia-feedstock that referenced this pull request Sep 13, 2017
* Removed wrapper script

* Bumped build number

* Use juliarc ( dfornika#1 )

* Add a custom juliarc.jl

Needed to ensure packages and other relevant information to the Julia
install are tracked in the conda environment.

* Install custom juliarc

Make sure that that our custom juliarc is available in the standard
location.

* Create the package directory

Make sure that Julia initializes our package directory and includes the
standard packages in it.

* Bump build number to 1

Needed to repackage after removal of the wrapper script, inclusion of
juliarc, and inclusion of the prepopulated package directory.

* Removed JULIA_HISTORY setting

* Drop package initialization ( dfornika#2 )

This does not work correctly as `conda-build` strips out the `.git`
directory from `METADATA`. There are no clean fixes for this issue. As
we already try to initialize the package directory when Julia starts,
this will be handled anyways on the user's machine. In the end, this may
be best to avoid having a potentially out-of-date index in the package.

* Append juliarc.jl instead of replace

* Append juliarc.jl the right way

* Added post-link script for Pkg.init()

* Removed Pkg.init() from juliarc.jl

* Correct setting Base.LOAD_CACHE_PATH

* Correct setting Base.LOAD_CACHE_PATH again

(cherry picked from commit 19d96fa)
dfornika pushed a commit that referenced this pull request Sep 13, 2017
* Removed wrapper script

* Bumped build number

* Use juliarc ( dfornika#1 )

* Add a custom juliarc.jl

Needed to ensure packages and other relevant information to the Julia
install are tracked in the conda environment.

* Install custom juliarc

Make sure that that our custom juliarc is available in the standard
location.

* Create the package directory

Make sure that Julia initializes our package directory and includes the
standard packages in it.

* Bump build number to 1

Needed to repackage after removal of the wrapper script, inclusion of
juliarc, and inclusion of the prepopulated package directory.

* Removed JULIA_HISTORY setting

* Drop package initialization ( dfornika#2 )

This does not work correctly as `conda-build` strips out the `.git`
directory from `METADATA`. There are no clean fixes for this issue. As
we already try to initialize the package directory when Julia starts,
this will be handled anyways on the user's machine. In the end, this may
be best to avoid having a potentially out-of-date index in the package.

* Append juliarc.jl instead of replace

* Append juliarc.jl the right way

* Added post-link script for Pkg.init()

* Removed Pkg.init() from juliarc.jl

* Correct setting Base.LOAD_CACHE_PATH

* Correct setting Base.LOAD_CACHE_PATH again

(cherry picked from commit 19d96fa)
This was referenced Sep 13, 2017
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