Skip to content

Conversation

@ericphanson
Copy link
Contributor

This PR fixes three issues I ran into when trying to use pre-commit's native Julia language support for ExplicitImports.jl (JuliaTesting/ExplicitImports.jl#100) in Pkg (JuliaLang/Pkg.jl#4326):

  1. Adds --startup-file=no to both invocations of Julia (install & run). The startup file is a Julia script that users can configure to run commands before the start of their Julia session. It is typically used to load developer packages like Revise and not used "in-production". Here when we run our pre-commit hooks we do not want to run the user's startup file which could have dependencies the current environment does not have.
  2. Copies src files in addition to Project/Manifest. This allows pre-commit hooks to live inside a package (like https://github.com/JuliaTesting/ExplicitImports.jl/blob/main/.pre-commit-hooks.yaml) instead of being in a separate repo (like https://github.com/fredrikekre/runic-pre-commit).
  3. Adds basic support for customizing the Julia version by setting the JULIAUP_CHANNEL environmental variable. This support is limited in a couple ways:
    • We don't check that the user is using juliaup. They could have installed julia some other way in which case the JULIAUP_CHANNEL does not do anything and setting the language_version will not have any effect. That however is the status quo (currently setting the language_version does not have any effect) and many users use juliaup, so I think we could just document that language_version support requires your julia to be from juliaup.
    • If the user passes a channel which is not installed, they will get a juliaup error that the channel can not be found. If we were confident they were using juliaup we could install it on their behalf instead, but I'm not sure this is necessary.

With these changes, JuliaLang/Pkg.jl#4329 can run successfully.

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

rather than doing 3 or 4 things in a pr please split this up to do one thing at a time with clear tests demonstrating the reason for changes

)
]
if version not in ('system', 'default'):
patches.append(('JULIAUP_CHANNEL', version))
Copy link
Member

Choose a reason for hiding this comment

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

this isn't quite right. to properly support language_version pre-commit must install the language itself into the hook environment (and not outside of it such as in the juliaup managed directories)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, pre-commit wants an entire copy of julia installed into the hook environment directory? that's very unusual for julia, which uses content-addressed read-only files and packages to allow reuse across environments. It would also make pre-commit install much slower for julia hooks than it currently is.


def _make_src_hook(tmp_path, pkg_code, script_code):
# here we have a package with a src dir and a script dir
src_dir = tmp_path.joinpath('src')
Copy link
Member

Choose a reason for hiding this comment

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

feels like from this test this should get handled by the install process. the precedence of copying little bits out of this seems not great and we should find a way to do this step by step

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is in the tests, so we're setting up an example with test files which do get copied as part of the install process

@ericphanson
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants