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

feat: allow Linux dependencies to be specified via a file path #254

Merged

Conversation

kevinaboos
Copy link
Contributor

This makes it possible to dynamically generate the list of dependencies for Linux Debian and pacman package formats.

Fixed a small documentation bug in PacmanConfig.

Notes

We needed this feature in Project Robius to relieve the app dev from the burdens of:

  1. figuring out which dependencies their app binaries need.
  2. having to keep that list up to date when adding or removing dependencies from their app, which typically is only done at the Rust crate level (rather than the Linux native package/library level).

This changeset has been tested to work for us on Ubuntu LTS 20.04, 22.04, and 24.04, on both aarch64 and x86_64.

Question: I modified the crates/packager/schema.json file by hand -- not sure if that's correct or if it's generated automatically. It appears that bindings/packager/nodejs/schema.json was indeed auto-generated; not sure if that needs to be committed too. Please advise, thanks!

This makes it possible to dynamically generate the list of dependencies for
Linux Debian and pacman package formats.

Fixed a small documentation bug in `PacmanConfig`.
Comment on lines +312 to +320
/// A path to the file containing the list of dependences, formatted as one per line:
/// ```text
/// libc6
/// libxcursor1
/// libdbus-1-3
/// libasyncns0
/// ...
/// ```
Path(PathBuf),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a bit unconventional, how about adding support for specifying dependencies as environment variables, CARGO_PACKAGER_DEB_DEPS="dep1,dep2,dep3" and CARGO_PACKAGER_PACMAN_DEPS="dep1,dep2,dep3"?

Copy link
Contributor Author

@kevinaboos kevinaboos Jul 2, 2024

Choose a reason for hiding this comment

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

Sure, a pair of env vars would also work, especially for simple cases like the one I gave in the rustdoc above. I can add that as well.

Would it be alright if we kept the file path option in there? There are a few reasons that come to mind, some from personal experience:

  • Unix utils tend to interop more easily with files; env vars are harder to access & modify by comparison.
  • Files are useful as intermediary artifacts, making it easy to manually inspect & modify them if needed in between various packaging steps
    • For example, if you have a set of different binaries in a single app, appending things to a file, deduplicating lines in a file, or many other operations are easier than doing so in an env var
  • It can be tough to specify complex dependency conditions while using the right quotation & special char escaping syntax to marshal them properly into env vars
    • To clarify, if you have a complex dependency that uses special characters like [, *, ~, !, etc, then it's easy to have a shell environment accidentally misinterpret those characters (especially for non-expert users), and especially when multiple different shell environments may be in use by various team members or build systems
      • Some examples of weird dependency strings are: openjdk-7-jre-headless (>= 7~u3-2.1.1), libc6 (>= 2.2.1), default-mta | mail-transport-agent,foo [linux-any], bar [any-i386], baz [!linux-any], libssl1.0.0_1.0.1t-1+deb8u6_amd64.deb etc. These are all quite a bit easier to express in a plaintext file.
  • Setting env vars differs across host OSes and can be (mildly) difficult to deal with in an abstract manner, and some runtime environments (chroots and the like) may clear or override env vars, which can be hard to debug.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for explanation, let's go with your suggestion using a file

@amr-crabnebula
Copy link
Collaborator

Question: I modified the crates/packager/schema.json file by hand -- not sure if that's correct or if it's generated automatically. It appears that bindings/packager/nodejs/schema.json was indeed auto-generated; not sure if that needs to be committed too. Please advise, thanks!

It should be automatically generated if your IDE runs the build script of config-schema-generator crate, if not, you can manually build it using cargo b -p config-schema-generator and it will generate the schemas.

@kevinaboos
Copy link
Contributor Author

ah sorry, i had forgotten to run rustfmt. Fixed now.

@amr-crabnebula amr-crabnebula merged commit c6207bb into crabnebula-dev:main Jul 2, 2024
22 checks passed
@amr-crabnebula
Copy link
Collaborator

Thank you

@kevinaboos
Copy link
Contributor Author

Thanks! Shall I go ahead and add env var support too?

@amr-crabnebula
Copy link
Collaborator

That would be awesome, thanks

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.

2 participants