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(server) Slightly Improved Linux Support #302

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mattlennon3
Copy link

What type of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactor
  • Documentation
  • Not Sure?

Does this PR introduce breaking changes?

  • Yes
  • No
  • Maybe

List any relevant issue numbers:

Description:

  • Installed getos package to better detect linux distributions.
  • Changed some package manager commands to reflect other distros.
  • Only adds Arch support atm but has room for others

Problems:

  • I updated the unit tests for os.js, when the test file is ran on it's own. It passes all tests. When running all unit tests it doesn't run this file at all, showing 0 coverage for os.js 🤔
  • I can only test this on arch. Will require someone to check things still work on Mac & Windows

Related Issue:
#141 ( I would love to add fedora support but I don't know what packages are required )

@@ -33,8 +33,21 @@ export default () => new Promise(async (resolve, reject) => {
await execa('curl', ['--version'])
log.success('"curl" found')
} else if (info.type === 'linux') {
await execa('apt-get', ['--version'])
log.success('"apt-get" found')
switch (info.distro) {
Copy link

@sleroq sleroq Jan 27, 2022

Choose a reason for hiding this comment

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

Given the number of different distributions, I think it would be easier to to just try the most popular package managers.

Maybe try something like this?

async function checkPM(name){
    await execa(name, ['--version'])
    return 'name'
}

let packageManager;
try {
    await Promise.any([
        checkPM('pacman'),
        checkPM('apt-get'),
        checkPM('dnf'),
    ])
} catch (error) {
    throw new Error('Unknown package manager')
}
log.success(`${packageManager} found`)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah definitely an improvement! I'll get around to fixing that, thanks. I don't suppose you had the same issue with running the test suite, what I mentioned in the PR description?

Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and your help, we appreciate it! @mattlennon3
Excuse us for the late reply.

Being able to run Leon on multiple CPU architectures, distribution, it's definitely something we would like to support, it's on the
Trello roadmap - 🔧 [1.0.0] Compatibility with ARM architectures and GNU/Linux distributions.
However, it's still something, we need to investigate to do it well :

  • Where to do it: in the CLI, in the core?
  • How many package managers we should support (and which ones)?
    etc.

As per this comment #302 (comment), I agree that we should not support "per different distribution", but instead per the most popular package manager, as usually adding support for a Linux distribution, is a matter of using the right package manager.

There is already something, we try to do in leon-cli :

https://github.com/leon-ai/leon-cli/blob/9cacbf5d6587524a8b4f2fefb6da2bd1d837c01c/src/services/Requirements.ts#L99-L129

But this is not complete, and we still need to change the core to not use apt-get but be compatible with more package managers. 👍
So this is still a work in progress, and we will need to figure it out before releasing the first stable release of Leon v1.0.0.

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 this pull request may close these issues.

3 participants