Fix remote modules when base dir is not the current working dir#6932
Fix remote modules when base dir is not the current working dir#6932bentsherman merged 6 commits intomasterfrom
Conversation
Signed-off-by: jorgee <jorge.ejarque@seqera.io>
✅ Deploy Preview for nextflow-docs-staging canceled.
|
pditommaso
left a comment
There was a problem hiding this comment.
Review
Good change — removing baseDir from the SPI is the right call since the old nf-lang caller was passing a wrong value (Path.of("./modules").getParent() resolves to .). The callers in ModuleResolver.java and ResolveIncludeVisitor.java are nicely simplified.
1. FallbackRemoteModuleResolver — relative path
Path.of("modules") is relative to CWD, which is fragile. This is acceptable for a fallback, but worth a comment explaining the intent. Also, consider using Path.of("modules").toAbsolutePath() for better diagnostics if it fails.
2. DefaultRemoteModuleResolver — null session fallback
def baseDir = Global.session?.baseDir ?: Path.of('.')When Global.session is null (e.g. nextflow module install before session creation, or testing), this falls back to a relative Path.of('.'). Consider Path.of('.').toAbsolutePath() to at least make the resolved path absolute for better error messages.
3. ResolveIncludeVisitor — unhandled exception from resolver
ResolveIncludeVisitor now calls RemoteModuleResolverProvider.getInstance().resolve(source) which can throw IllegalStateException from the fallback resolver when the module isn't locally installed. This exception doesn't appear to be caught — it could crash the language server / IDE integration. Consider catching it and reporting as a diagnostic error instead.
4. Naming simplification
Consider renaming for clarity:
RemoteModuleResolverProvider→RemoteModuleResolver(it's the provider/factory, the current name is redundant)FallbackRemoteModuleResolver→DefaultModuleResolver(simpler, clearer role)ModuleResolver(in nf-lang) → keep as-is or clarify its relationship to the SPI
5. Log message verbosity
log.info "Module ${reference}@${version} installed successfully at ${installed.mainFile.parent.toAbsolutePath().toString()}"The .toString() is redundant in GString (Groovy calls it automatically). Also, the chain installed.mainFile.parent.toAbsolutePath().toString() is quite long inline. Consider a small helper or local variable:
def installDir = installed.mainFile.parent.toAbsolutePath()
log.info "Module ${reference}@${version} installed successfully at ${installDir}"Or a utility like Path.toUriString() if this pattern repeats elsewhere.
|
Thanks Jorge. Let me look at this with the linter and language server. We might need to be able to pass in the projectDir (baseDir) in those cases |
Signed-off-by: Jorge Ejarque <jorgee@users.noreply.github.com>
Signed-off-by: jorgee <jorge.ejarque@seqera.io>
|
Addressed review comment. I skipped the suggested renaming as it collides with other existing classes |
Signed-off-by: jorgee <jorge.ejarque@seqera.io>
This pull request refactors how remote module resolution works at parsing and compile time. The
RemoteModuleResolverinterface is updated to remove the need to explicitly pass a base directory.At pipeline execution modules are resolved according to the basedir. Otherwise, it uses "./modules" as resolution base dir. This fixes the problems when invoking remote pipelines with modules or executing a pipeline with a different path.
A pipeline including remote modules is created to test this behaviour.
In the main branch, it uses remote modules without pinned versions. You can run with the following command
./launch.sh run jorgee/nf-modules-testThe first time you run this pipeline, it will install the modules in the version assets folder (execution basedir)
It also contains the
pinned-versionsbranch where the remote module is pinned (commited files). So, modules shouldn't be installed at execution in this case./launch.sh run jorgee/nf-modules-test -r pinned-versions