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

chore: consolidate and add a collection of swingset analysis tools #3789

Merged
merged 3 commits into from
Sep 22, 2021

Conversation

warner
Copy link
Member

@warner warner commented Sep 2, 2021

cc @dckc here are some tools that may or may not be helpful

@warner warner added the SwingSet package: SwingSet label Sep 2, 2021
@warner warner requested a review from FUDCo September 2, 2021 21:25
@warner warner self-assigned this Sep 2, 2021
@@ -0,0 +1,52 @@
import '@agoric/install-ses';
import process from 'process';
import { openLMDBSwingStore } from '@agoric/swing-store-lmdb';
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to have been cribbed from one of the db tools as they existed before the recent swing store refactoring.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

A couple of the files predate the swing store refactoring, but you've probably already noticed that because they won't pass CI.

The GitHub PR machinery doesn't support commenting on file renaming so I'll comment here: A couple of the tools that you moved into analysis-tools from tools aren't analysis tools, notably rekernalize and replace-bundle. Some of the database tools aren't really used for analysis either, but for making changes and repairs. We should either go back to just having a single tools directory or be more selective about what we put where.

I can't comment as to the awesomeness/adequacy of the Python tools :-)

@warner
Copy link
Member Author

warner commented Sep 2, 2021

Oh, yeah, there's no CI on any of this. Although I suppose I have to make lint stop complaining, at least.

I figured that rekernelize and replace-bundle qualified as "ad-hoc", since we built them to investigate a low-level XS memory problem, and normal "users" of swingset wouldn't need them. I kind of wanted to put all the not-for-normal-use things in a single directory, and bin/ didn't seem right (I think of bin/ as what would get installed into /usr/bin/ if someone were to install a swingset package from APT or RPM or NPM or whatever), and tools/ is currently occupied (infested?) by importable SES/AVA things, which discourages me from putting executable/invokable things in the same place. So I figured I'd just move everything into a new directory.

Maybe the answer is to change the analysis part of the analysis-tools name. If I had my druthers I'd name it tools and move the importable SES/AVA things elsewhere, but that's a lot more work now that the rest of the agoric-sdk tree is doing a deep import.

@dckc
Copy link
Member

dckc commented Sep 3, 2021

Thanks for sharing this stuff. I'm mildly surprised to see a non-DRAFT PR, i.e. aimed for long-term maintenance. Is that really the target?

@warner
Copy link
Member Author

warner commented Sep 3, 2021

Thanks for sharing this stuff. I'm mildly surprised to see a non-DRAFT PR, i.e. aimed for long-term maintenance. Is that really the target?

This PR is just about getting a bunch of random tools into a place where you (and others) can access them. Half of them barely even work, they're ad-hoc things I whipped up, and are hyper-specific to the particular bit of investigation I was doing. I figure that putting them in a suitably-misc-ish directory and mentioning their non-polished nature in a README is sufficient, and we (I) shouldn't spend any time trying to make them official-looking. Commit early, commit often, if it's not part of the executing code then it doesn't have to be perfect.

I'm going to rename the directory misc-tools and put back the misc tools that were previously in bin and tools, even though I don't think they deserve to be there (not in bin/ because I expect things there to be polished and useful to end-users, as opposed to us kernel-bug/chain-performance investigators, and not in tools/ because that's been stolen by deep-importable things for SES/AVA testing in other packages).

@dckc
Copy link
Member

dckc commented Sep 3, 2021

...
This PR is just about getting a bunch of random tools into a place where you (and others) can access them.

I guess I'm saying: that's achieved already, without merging anything.

@warner
Copy link
Member Author

warner commented Sep 14, 2021

I guess I'm saying: that's achieved already, without merging anything.

Fair point, I guess my real concern is that I'll keep losing them (and others won't be able to discover them) unless they're in the tree for real.

@warner warner force-pushed the swingset-tools branch 2 times, most recently from 9ef01bb to 318d9ca Compare September 15, 2021 02:17
Some were previously in bin/ , however they aren't really solid enough to
live there. Some were in tools/, however that's currently the home of
importable utilities like `prepare-test-env-ava.js`, and I want to avoid
confusion between "things you run" and "things you import". Some are new,
assembled from various one-off analysis tools I've written over the last
year.

These are all getting put in misc-tools/ , and are excluded from lint and
prettier, as befits quick hacks.
Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

Yah shoor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants