-
Notifications
You must be signed in to change notification settings - Fork 76
Add options for the Julia binary and project path #667
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
Conversation
This looks good thank you! A few comments:
|
Roger that, I think all of those are fixed in 69b90ca (tested again manually). (BTW I like using fixup commits during review :) if you think this is ready then I can rebase the branch, or you can squash merge) |
Don't worry, I always squash. Looking good. Main other thing is testing. If you're familiar with GitHub CI can you add a case to the test workflow that uses these new env vars? The PythonCall tests (in the same file) do a similar thing to test against either CondaPkg or a pre-installed python. You'll have to conditionally do the install-julia action, followed by running a command to set up a Julia project, and then set these env vars when running the tests. If you're unsure I can do it when I get some time. |
This makes using JuliaPkg optional.
636aaf4
to
22688ee
Compare
Got there in the end 😅 I tried to keep the actual test command contained in one step but it was tricky to set the environment variables correctly so I split it into one step for JuliaPkg and another without. Turns out that CI was already running the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good just a couple of small tweaks please.
I made a few little tweaks before merging. Great work, thank you! |
This makes using JuliaPkg optional. Also sneaked in a change to use
[sources]
in the docs project.Tested manually. Should fix JuliaPy/pyjuliapkg#56.