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

[WIP] Find julia installation #100

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jlapeyre
Copy link

@jlapeyre jlapeyre commented Dec 15, 2021

This PR adds get_installed_bin_path(preferred_versions=None, only_preferred=False).

It may be called like this

get_installed_bin_path(preferred_versions=['1.6', '1.7', 'latest'], only_preferred=False)

to find the path the the exectuable of the first preferred Julia version found, or otherwise, the first Julia version found.

See #99

This allows searchin for an installed Julia exectuable.
@jlapeyre
Copy link
Author

I am using this approach in julia_project.

This file is a copy of the code in this PR. I intend to remove it when this PR, or something like it, is merged into jill.py

Copy link
Owner

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Looks good... Just a few niche-picking comments.

Could you add a few lines of tests and insert them after

- name: test list and switch
run: |
coverage run -a -m jill list
coverage run -a -m jill switch 1.0
python -m jill list
coverage run -a -m jill switch 1
python -m jill list
coverage run -a -m jill switch 1.0 --target julia-1
python -m jill list
?

jill/install.py Outdated Show resolved Hide resolved
jill/install.py Outdated Show resolved Hide resolved
jill/install.py Outdated Show resolved Hide resolved
jill/install.py Outdated
Comment on lines 41 to 45
found_installation = None
for preferred_installation in preferred_julia_installations:
if preferred_installation in installed_julias:
found_installation = preferred_installation
break
Copy link
Owner

Choose a reason for hiding this comment

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

This is niche picking so it's okay to keep the current version; I personally feel it's better to return ALL paths that satisfy the requirement and let the callers decide which one is needed.

So for caller, it's perhaps easier to understand this way:

julia_paths = get_installed_bin_path(...)
if not julia_paths:
    raise ValueError("Failed to found julia executables")
preferred_julia_path = julia_paths[0]

Copy link
Author

Choose a reason for hiding this comment

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

Good idea. But, the versions will be buried in the path to the executable. I'll push a version that returns a dict of executable paths keyed on version. See what you think of that.

@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #100 (75770a9) into master (3b2a29e) will decrease coverage by 1.28%.
The diff coverage is 8.82%.

❗ Current head 75770a9 differs from pull request most recent head 38a732b. Consider uploading reports for the commit 38a732b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
- Coverage   61.00%   59.72%   -1.29%     
==========================================
  Files          16       16              
  Lines        1349     1383      +34     
  Branches      254      292      +38     
==========================================
+ Hits          823      826       +3     
- Misses        431      462      +31     
  Partials       95       95              
Impacted Files Coverage Δ
jill/install.py 38.54% <8.82%> (-4.20%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b2a29e...38a732b. Read the comment docs.

@jlapeyre
Copy link
Author

The last commit takes care of some issues. I have not yet added tests.

Copy link
Owner

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Nice! Much shorter and easier to read; with some basic test coverage I think we can get this in.

Return a dict whose keys are Julia version strings and whose values are
absolute paths to the corresponding Julia exectuable. Only jill.py's
default directory for Julia installations is searched.
The version strings either equal to `latest` or have the form `major.minor`.
Copy link
Owner

Choose a reason for hiding this comment

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

If my memory serves well, there can be a third case julia-dev if you install the dev version Julia via jill install 1.8.0+848d273.

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.

2 participants