-
Notifications
You must be signed in to change notification settings - Fork 63
Automate the install process in the README #856
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
Conversation
tools/install_prerequisites.sh
Outdated
| 'library/postgresql-13' | ||
| 'pkg-config' | ||
| 'brand/omicron1/tools' | ||
| 'pkg:/package/pkg' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this happen first, and maybe unconditionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can make it the first install.
I've also just updated the tooling here to make the call to pkg install unconditionally, which should act as both "update" and "install" - I'll just check the error code "no updates to install (4)", and explicitly allow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source on the usage of "4", since it's a little magic:
https://man.omnios.org/man1/pkg.1#:~:text=No%20changes%20were%20made%20%2D%20nothing%20to%20do.
bnaecker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK I think, just one question about the order of installing pkg itself.
davepacheco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry I wasn't able to make it to Omicronathon! I imagine this came out of that and I'm missing context on it. So take with a grain of salt.
Also: while I've got reservations about this approach, I'm comfortable with folks saying this is worthwhile as-is.
Okay, disclaimers aside: I get nervous about scripts like this. My experience is that they tend to be run only occasionally -- usually, when a new member has joined the team. So they're often out of date when they're run and they don't quite work. They're often not idempotent, or that's not tested regularly so that part doesn't work, or as a user I don't know if it's supposed to be idempotent so I have to read the source anyway -- I'd rather read the README. Anything that installs packages has to assume a lot about the environment, which means it necessarily can only work in some environments. Being bash scripts, they tend to fail in non-obvious ways. My feeling is usually that broken automation is worse than manual steps (even if the docs are outdated) and this activity isn't done often enough to be worth automating well.
I think when these were README steps, the responsibility was more clearly on the reader to figure this stuff out. By automating it, I think we're implicitly taking on some of that responsibility.
Okay, so those are my fears. We could mitigate most of this by:
- Automatically testing it. Could we update our automatic build and test runners to start from a more stripped-down environment (instead of the kitchen sink that GitHub normally starts these things in) and use this script to install the prerequisites? Then at least we'd know right away if it was broken on any of the three platforms it supports.
- Automatically testing idempotence -- I don't know how to do this.
- Checking for more conditions that could reasonably trip people up. This isn't a binary thing -- I'm not sure how far to go. But conditions I can imagine include:
- Don't have sudo/pfexec set up (or: don't have/need sudo/pfexec -- but I don't know how you can tell if it's not needed)
- On Linux, but don't have
apt-get
- Spitting out a message at the end on failure that explicitly says something like "Something went wrong. This script is idempotent. If you can fix the underlying problem, re-run the script to finish installing dependencies."
It'd also be nice to better communicate what the script is going to do. We might want to tell them we're going to run sudo, for example, so they're not sketched out by getting a password prompt. Will the package manager commands prompt the user for confirmation? If so, at least they'll have a chance to see what other mess of packages will be installed (or uninstalled if there are conflicts?) If not, maybe an explicit confirmation prompt in the script would be worthwhile.
This was the core motivator - I saw lots of folks running through the commands in the README, things not working correctly, and taking a chunk of time to repair. Most common issues:
Additionally, I personally felt this pain when trying to get up-and-running on lab machines. Often, each one has slightly different setups, slightly different pre-installed software, and being able to automate the setup would have saved me a chunk of time.
I agree that the responsibility was clearly on the reader in this case, but that also made the burden high.
I'd like this. Toying around with this here: #870
I'd really like this - I want people to just be able to re-run this script at any time to make their prebuilts "up-to-date" if anything changes. This was definitely my intent while writing it.
I'm not sure how to test for these conditions, but I'd be happy to make the error messages surrounding them more clear.
Added!
Added a confirmation prompt. |
|
Is there anything else I can do to make this change more palatable? Should we be opening this up to discussion to the team as a whole? |
|
That context is helpful. I remain worried about the things I mentioned, but understanding that this sounds useful to people now, let's go ahead and try it. |
Changes since the last update: - lib: use correct MAXCPU value in CPUID specializer (#876) - phd: wait for source to resume before asking to migrate again (#874) - phd: add smoke test for VCR replacement (#872) - lib: implement reference TSC enlightenment (#856) - Update package deps for GHSAs - Wire up viona for illumos#17032 - mock-server: add single-step API (#869) - propolis-server should not crash when failing to start a VM (#866) - propolis-cli: check for duplicate spec keys when parsing toml (#865) - various new 1.85 clippy lints (#864) - mock: attempt realistic state transitions (#860) - lib: tidy up overlay page migration & reduce memory usage (#861) - server: add state machine docs (#862) - DTrace script to inspect VM exit reasons (#859) - lib: add better management of Hyper-V overlay pages (#851) - lib: emulate Hyper-V enlightenment stack (#849)
No description provided.