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

Inconsistency between endo vs agoric-sdk on syntax of DEBUG options #8096

Closed
erights opened this issue Jul 25, 2023 · 2 comments · Fixed by #8136
Closed

Inconsistency between endo vs agoric-sdk on syntax of DEBUG options #8096

erights opened this issue Jul 25, 2023 · 2 comments · Fixed by #8136
Assignees

Comments

@erights
Copy link
Member

erights commented Jul 25, 2023

agoric-sdk says

const LABEL_INSTANCES = (env.DEBUG || '')
.split(':')
.includes('label-instances');

whereas endo says
https://github.com/endojs/endo/blob/ac9bb3b03bfa67fdbbaf9a6febde1fba7ba154da/packages/exo/src/exo-makers.js#L14

const LABEL_INSTANCES = DEBUG.split(',').includes('label-instances');

Thus, one splits on commas and the other splits on colons. IIRC, originally both agreed on the wrong answer. But it seems the correction to the right answer happened only in one place, leaving them inconsistent.

As to what the right answer is, grepping for DEBUG yielded some more surprises:

} else if (DEBUG.includes('agoric')) {

tests only for string inclusion, so setting DEBUG=noagoric will mean the same as DEBUG=agoric.

pspawnEnv.DEBUG = 'agoric,SwingSet:vat,SwingSet:ls';

suggests that our convention is that the outer separator is comma, and that colon is used within each comma-separated segment for something like componentName:optionName. If that is correct, then the endo comma for label-instances is correct and the agoric-sdk code is buggy. Hence, I'm filing this as an agoric-sdk bug. But in that case, the endo code

https://github.com/endojs/endo/blob/ac9bb3b03bfa67fdbbaf9a6febde1fba7ba154da/packages/eventual-send/src/track-turns.js#L23

const VERBOSE = DEBUG.split(':').includes('track-turns');

is buggy.


Related:

We can now switch agoric-sdk over to use @endo/env-options rather that rolling our own.

Controversial: One of us thinks that...

We should whenever possible avoid inventing yet another textual language that needs to be parsed accurately. This leads exactly to the hazards we fell into above. By contrast, the environment variables for the lockdown options are separate variables per option, each prefixed with LOCKDOWN_. We should deprecate DEBUG entirely and shift to this env-var-per-option with common prefix convention instead,

@erights
Copy link
Member Author

erights commented Jul 26, 2023

@erights
Copy link
Member Author

erights commented Aug 29, 2023

See also nodejs/node#48890

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 a pull request may close this issue.

2 participants