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

fix: DEBUG harmony #8136

Merged
merged 7 commits into from
Feb 6, 2024
Merged

fix: DEBUG harmony #8136

merged 7 commits into from
Feb 6, 2024

Conversation

erights
Copy link
Member

@erights erights commented Aug 2, 2023

closes: #8096

Updates agoric-sdk to use the new conveniences exported by @endo/env-options.

Reform the contents of the DEBUG environment variable, i.e., the process.env.DEBUG option to be a better behaved list for agoric:* options. See the new docs/env.md for details.

NOTE: This reform of the meaning of the agoric:* members of the DEBUG list is in theory a compat issue. But because we have no known occurrences of that, and it would only affect debugging and logging behavior anyway, we do not flag this PR as a breaking change.

docs/env.md Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-8096-DEBUG-harmony branch 3 times, most recently from 1284f64 to 641d38e Compare August 11, 2023 04:48
@erights erights changed the base branch from master to markm-8231-interfaces-be-passable August 22, 2023 02:42
@erights erights changed the base branch from markm-8231-interfaces-be-passable to master August 28, 2023 05:57
@erights
Copy link
Member Author

erights commented Aug 29, 2023

See also nodejs/node#48890

@michaelfig
Copy link
Member

nvm my earlier (deleted) comment about the CI failure. It seems a perfect specimen of the #endo-branch: failures we were seeing on other PRs, so I will try to fix it in this PR.

@michaelfig michaelfig force-pushed the markm-8096-DEBUG-harmony branch 5 times, most recently from 94c6cbf to 531eb51 Compare January 7, 2024 04:17
@erights erights changed the base branch from mfig-build-uses-hashes to mark-rebaseof-mfig-build-uses-hashes January 29, 2024 08:24
@erights erights changed the base branch from mark-rebaseof-mfig-build-uses-hashes to master January 30, 2024 22:05
@erights erights force-pushed the markm-8096-DEBUG-harmony branch 2 times, most recently from fa82f4b to 9a47f38 Compare February 3, 2024 04:53
@erights erights changed the title WIP fix: DEBUG harmony fix: DEBUG harmony Feb 3, 2024
@erights
Copy link
Member Author

erights commented Feb 3, 2024

Hi @michaelfig , this is Ready for Review.

@erights erights marked this pull request as ready for review February 3, 2024 05:00
@erights
Copy link
Member Author

erights commented Feb 3, 2024

@michaelfig , could you take a look at the failing integration tests? Thanks.

@michaelfig
Copy link
Member

could you take a look at the failing integration tests?

This has been bumped up in my queue. I ground to a halt the last time I tried; with fresh eyes I should get it done this week.

@michaelfig
Copy link
Member

@erights, I pushed some changes to restore some of the logging default behaviour. This was to cause silencing (using the info level as the minimum default) only if $DEBUG was explicitly set to the empty string. Not setting $DEBUG at all, or setting it to anything non-empty, would use the log level as the minimum default.

Let's see this pass CI, then I will review again.

@erights
Copy link
Member Author

erights commented Feb 5, 2024

Let's see this pass CI, then I will review again.

It passed, thanks much!

@erights erights force-pushed the markm-8096-DEBUG-harmony branch 2 times, most recently from 7de259b to afcb935 Compare February 5, 2024 22:31
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Please address comments, but otherwise this LGTM!

docs/env.md Outdated
@@ -97,6 +102,10 @@ Description: When nonempty, create pretend prepopulated tokens like "moola" and

Lifetime: until chain is mature enough not to need any pretend tokens

## LOCKDOWN_*

For the envoronment variables beginning with `LOCKDOWN_` , see [`lockdown` Options](https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For the envoronment variables beginning with `LOCKDOWN_` , see [`lockdown` Options](https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md).
For the environment variables beginning with `LOCKDOWN_` , see [`lockdown` Options](https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md).

Comment on lines 379 to 382
} else if (DEBUG_LIST.includes('agoric:info')) {
verbosity = ['-v'];
} else {
verbosity = [];
Copy link
Member

Choose a reason for hiding this comment

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

I overlooked changing this. Please update:

Suggested change
} else if (DEBUG_LIST.includes('agoric:info')) {
verbosity = ['-v'];
} else {
verbosity = [];
} else if (DEBUG_LIST.includes('agoric:info') || process.env.DEBUG='') {
verbosity = [];
} else {
verbosity = ['-v'];

@erights erights added the automerge:squash Automatically squash merge label Feb 6, 2024
@mergify mergify bot merged commit d2ea4b4 into master Feb 6, 2024
70 of 77 checks passed
@mergify mergify bot deleted the markm-8096-DEBUG-harmony branch February 6, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistency between endo vs agoric-sdk on syntax of DEBUG options
2 participants