-
Notifications
You must be signed in to change notification settings - Fork 15
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
Improve linux dependency check #195
base: main
Are you sure you want to change the base?
Improve linux dependency check #195
Conversation
linux: 'linux', | ||
lin: 'linux', | ||
linux: 'lin', | ||
lin: 'lin', |
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.
When I changed this in #190, it actually broke some of the later steps during setup that I hadn't anticipated. It turns out, what needed to change was the actual platform file name, and I did so in one of these commits.
src/toolbox/setup/lin.ts
Outdated
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.
Renaming this file is what actually fixes the broken import line in the commands/setup.ts script.
I'm curious if the same issue is happening on the Windows side of things, since the device alias is win but the import filename is actually windows.ts. I don't have a Windows machine to check this, though.
export async function installPackages(packages: Dependency[]): Promise<void> { | ||
const packageManager = system.which('apt') | ||
|
||
if (packageManager !== null && packageManager !== undefined) { | ||
await execWithSudo( | ||
`${packageManager} install --yes ${packages.map((p) => p.packageName).join(' ')}`, | ||
{ stdout: process.stdout }, | ||
) | ||
} else { | ||
print.warning( | ||
'xs-dev attempted to install dependencies, but your Linux distribution is not yet supported', | ||
) | ||
print.warning( | ||
`Please install these dependencies before running this command again: ${packages.map((p) => p.packageName).join(', ')}`, | ||
) | ||
process.exit(1) | ||
} |
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.
This is a handy abstraction. I wonder if it's worth exploring integration something like UPT on top of this as a follow up. 🤔
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'll have to read up on it, but I like the idea of letting another tool do all the work for us.
I attempted to add some structure to the system package dependency installation on Linux here, and I would love some feedback. I realize this steps out ahead of #120 with a more brute-force approach to declaring dependencies, but my primary concern here is adding some logic around dependency checking as mentioned in #193
It is fairly common for Linux devs to know how to ensure that dependencies are installed on their system. This code change enables Linux devs on any Linux distribution to see that their package manager is not supported and know exactly which dependencies are missing when running
setup
,setup linux
,setup esp32
, andsetup esp8266
. Once they have seen what is missing, they can choose to install the libraries and/or binaries necessary to run the setup again.I have not attempted to modify
pico
orwasm
, because they include thebuild-essential
meta package for Debian. While this is a convenient catchall, it would be more cross-platform friendly to know exactly what tools/libraries are needed for each platform and check/install them individually. Any insight there would be helpful.