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

Remove wrapper script #11

Merged
merged 12 commits into from
Sep 13, 2017
Merged

Remove wrapper script #11

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c1ad13b
Removed wrapper script
dfornika Sep 12, 2017
0869a65
Bumped build number
dfornika Sep 12, 2017
6a8764e
Use juliarc (#1)
jakirkham Sep 12, 2017
34564f3
Merge branch 'remove-wrapper-script' of github.com:dfornika/julia-fee…
dfornika Sep 12, 2017
3336cbb
Removed JULIA_HISTORY setting
dfornika Sep 12, 2017
2562b2f
Drop package initialization (#2)
jakirkham Sep 12, 2017
1d475f7
Append juliarc.jl instead of replace
dfornika Sep 12, 2017
9dea820
Append juliarc.jl the right way
dfornika Sep 12, 2017
1a20b7d
Added post-link script for Pkg.init()
dfornika Sep 12, 2017
ceac1e1
Removed Pkg.init() from juliarc.jl
dfornika Sep 12, 2017
f00d2db
Correct setting Base.LOAD_CACHE_PATH
dfornika Sep 12, 2017
be02b37
Correct setting Base.LOAD_CACHE_PATH again
dfornika Sep 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions recipe/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ make -j 4 prefix=${PREFIX} MARCH=core2 sysconfigdir=${PREFIX}/etc NO_GIT=1 \
TAGGED_RELEASE_BANNER="conda-forge-julia release" \
install

mv "$PREFIX/bin/julia" "$PREFIX/bin/julia_"
cp "$RECIPE_DIR/julia-wrapper.sh" "$PREFIX/bin/julia"
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?


# 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.

24 changes: 0 additions & 24 deletions recipe/julia-wrapper.sh
View file
Open in desktop

This file was deleted.

12 changes: 12 additions & 0 deletions recipe/juliarc.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
JULIA_PREFIX = abspath(joinpath(Base.source_path(), "..", "..", ".."))

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()?

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()).

pop!(Base.LOAD_CACHE_PATH)
Copy link
Member

@jakirkham jakirkham Sep 12, 2017 •
edited
Loading

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.

end

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 •
edited
Loading

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.

2 changes: 1 addition & 1 deletion recipe/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ source:

build:
skip: True # [osx or win]
number: 0
number: 1
features:
- blas_{{ variant }}

Expand Down