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

Wrap plugin env in quotes #1684

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

dcdenu4
Copy link
Member

@dcdenu4 dcdenu4 commented Nov 11, 2024

Description

When testing a recent installation the plugin env path was being stored in the settings config without escaped quotes for path spacing. When running the plugin model I was seeing:

[11:57:18.970] [undefined] about to run model with command: "C:/Users/ddenu/AppData/Local/Programs/InVEST 3.14.2.post397+g9c664c177 Workbench/resources/miniforge3/condabin/mamba.bat" run,--prefix C:\Users\ddenu\AppData\Local\Programs\InVEST 3.14.2.post397+g9c664c177 Workbench\resources\Miniforge3\envs\invest_plugin_schistosomiasis,--live-stream,invest,-vv,--taskgraph-log-level=INFO,--language "en",run,schistosomiasis,-d "C:\Users\ddenu\AppData\Roaming\invest-workbench\tmp\data-Da2SMk\datastack.json"
[11:57:20.346] [undefined] Not a conda environment: C:\Users\ddenu\AppData\Local\Programs\InVEST

Wrapping the env path follows similar practices to how we store the invest and mamba paths.

Copy link
Member

@emlys emlys left a comment

Choose a reason for hiding this comment

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

Thanks @dcdenu4! IMO, it might be even better to wrap the path in quotes where it is used, instead of where it's stored, because that's where the quotes are needed. But this achieves the same thing so I'll leave it up to you

@@ -60,7 +60,7 @@ export function setupAddPlugin() {
pyname: pluginPyName,
type: 'plugin',
source: pluginURL,
env: envPath,
env: `\"{envPath}\"`,
Copy link
Member

Choose a reason for hiding this comment

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

Why do the quotes need to be escaped?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, because they're a special character in the config JSON.

@dcdenu4
Copy link
Member Author

dcdenu4 commented Nov 12, 2024

IMO, it might be even better to wrap the path in quotes where it is used, instead of where it's stored, because that's where the quotes are needed.

I initially went that route, but noticed in the config file we had stored investExe and mamba similarly.

{
	"nWorkers": -1,
	"taskgraphLoggingLevel": "INFO",
	"loggingLevel": "INFO",
	"language": "en",
	"investExe": "\"C:/Users/ddenu/AppData/Local/Programs/InVEST 3.14.2.post397+g9c664c177 Workbench/resources/invest/invest.exe\"",
	"mamba": "\"C:/Users/ddenu/AppData/Local/Programs/InVEST 3.14.2.post397+g9c664c177 Workbench/resources/miniforge3/condabin/mamba.bat\"",
	"core": {
		"port": 63911,
		"pid": 11296
	},

I like handling it on the code side as well though. I'll make that change and re-request.

@dcdenu4 dcdenu4 requested a review from emlys November 12, 2024 16:00
@emlys emlys merged commit 88126e4 into natcap:feature/plugins Nov 12, 2024
29 checks passed
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