Skip to content

Conversation

@chadlwilson
Copy link
Member

@chadlwilson chadlwilson commented Jan 14, 2023

This change replaces the Python+direct file reading hack for determining a "complete" OS family names with a more modern cross-platform library (oshi) that does the same. The library has ability to use native code to get a LOT more information out from the underlying OS than is being used here.

The hack was introduced in #2421 despite some of the initial objections (as to shelling out with Python, here), reverted, and then reintroduced in #2608 eventually. The attempt to shell out to Python happens regardless of OS, and I'm not sure it does any better than reading the name from /etc/os-release anyway.

This change also

  • moves the logic back to the agent lib, to avoid dependencies propagating everywhere. Only the agent needs this logic.

Some examples (before --> after)

Basically the same (linuxes)

  • Alpine Linux v3.17 --> Alpine Linux 3.17.1
  • Ubuntu 22.04.1 LTS --> Ubuntu 22.04.1 LTS (Jammy Jellyfish)
  • CentOS Stream 9 --> CentOS Stream 9
  • Debian GNU/Linux 11 (bullseye)--> Debian GNU/Linux 11 (bullseye)
  • Windows 11 --> Windows 11

Enhanced (non-linuxes)

  • Mac OS X --> macOS 13.1 (Ventura)
  • Windows Server 2022 --> Windows Server 2022 (Datacenter)

It also adds support for many others, so is far is less opinionated than before.

Task list

  • Prefix with manufacturer? (Linux, GNU/Linux, Apple, Microsoft etc)
    • The existing ones don't have a "manufacturer name" so let's leave it for now
  • Verify fallback behaviour of OSHI if can't determine at least the family + version

@chadlwilson chadlwilson added the java Pull requests that update Java code label Jan 14, 2023
@chadlwilson chadlwilson added this to the Release 23.1.0 milestone Jan 14, 2023
@chadlwilson chadlwilson force-pushed the simplify-os-family-checking branch 2 times, most recently from 08a0106 to 12be99f Compare January 14, 2023 16:58
@chadlwilson chadlwilson self-assigned this Jan 14, 2023
@chadlwilson chadlwilson force-pushed the simplify-os-family-checking branch 6 times, most recently from 1b73295 to 0917051 Compare January 15, 2023 16:47
@chadlwilson chadlwilson marked this pull request as ready for review January 15, 2023 17:07
@chadlwilson chadlwilson requested a review from arvindsv January 15, 2023 17:11
@chadlwilson chadlwilson changed the title Simplify OS family checking Simplify OS completeName determination Jan 15, 2023
@chadlwilson
Copy link
Member Author

Any input from Mr @ketan here would also be appreciated :-)

@ketan
Copy link
Member

ketan commented Jan 16, 2023

I remember seeing this library, but was not as evolved/matured at the time. The reason OperatingSystem.java exists in its current shape and form is to ensure that it always returns some "sensible" OS name and prevents startup failure. As long as you're able to prevent that startup failure, and checks most popular linux distros, go for it.

@ketan
Copy link
Member

ketan commented Jan 16, 2023

Also, I've not seen the code — but I'd make sure to "cache" the value, so as to not evaluate the OS version on every method invocation, mostly to avoid any expensive shelling out or file reading this library might be doing under the hood.

@chadlwilson
Copy link
Member Author

chadlwilson commented Jan 17, 2023

Thanks - yeah it caches the value. I'll double check the behaviour if the library can't read the value for some reason for a given distro/OS. (edit: added a fallback)

Copy link
Member

@arvindsv arvindsv left a comment

Choose a reason for hiding this comment

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

Looks good to me. The SystemInfoTest is the one I was worried about -- as not being future proof, and whether we needed such a strict regex to verify it. But, it's unlikely to change too quickly. Should be ok.

Also, TIL about !! used in build.gradle (gradle doc)

@chadlwilson
Copy link
Member Author

chadlwilson commented Jan 18, 2023

Also, TIL about !! used in build.gradle (gradle doc)

Probably would be better to use the more descriptive version than the magic. (edit: fixed)

The OSHI library depends on slf4j APi 2.x and it causes an implicit upgrade where which has some other problems I haven't been able to workaround yet with respect to the way we use classloaders within the agent and launcher. Their usage should still work with SLF4J 1.7.x as far as I can tell since it is largely API compatible.

@chadlwilson chadlwilson force-pushed the simplify-os-family-checking branch from 0917051 to f8e38d2 Compare January 18, 2023 02:40
…hi library

Replace with much more modern library which we can also use for other purposes, with much
better cross-platform support, and without relying on bizarre process forking to determine
details.
@chadlwilson chadlwilson force-pushed the simplify-os-family-checking branch from f8e38d2 to e20ecae Compare January 18, 2023 03:45
@chadlwilson chadlwilson merged commit ef2b792 into gocd:master Jan 18, 2023
@chadlwilson chadlwilson deleted the simplify-os-family-checking branch January 18, 2023 04:43
@chadlwilson chadlwilson changed the title Simplify OS completeName determination Simplify & improve OS completeName determination Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents java Pull requests that update Java code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants