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

Specify arbitrary container arguments #1315

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

har7an
Copy link
Contributor

@har7an har7an commented Aug 21, 2023

Implements the ability to pass arbitrary arguments to the underlying container runtime (podman/docker). This functionality is currently exposed:

  • in the config file, as build.env.extra_args
  • via environment-variables as EXTRA_ARGS

and takes a vector of strings. These strings are then injected into the container commandline.
This allows solving problems such as #1012 without having to wait for a new release of cross to handle every users needs.

The PR currently lacks tests and documentation. I wanted to put this up here to discuss the idea first before investing more time into it.

to container runtimes using a new configuration key under `build.env`.
This gives users the flexibility to work around issues such as cross-rs#1012
without having to wait for a new release or creating their own forks of
`cross`.
@har7an har7an requested a review from a team as a code owner August 21, 2023 13:37
har7an added 3 commits August 21, 2023 15:40
implemented in the configuration file and parsed from environment
variables.
by adding an empty default value for `extra_args` where required.
@har7an har7an force-pushed the feature/arbitrary-container-arguments branch from f51b2c4 to 1034191 Compare August 21, 2023 13:47
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

How is this different from CROSS_CONTAINER_OPTS defined here https://github.com/cross-rs/cross/blob/37c681a923407b67044ea09c40205012c0ea86d5/src/docker/shared.rs#L1057C45-L1057C45?

obviously this adds a way to specify it in the config file, but other than that I don't see any difference.

If it is the same, I'd prefer removing CROSS_EXTRA_ARGS and making CROSS_CONTAINER_OPTS available in config file.

@har7an
Copy link
Contributor Author

har7an commented Aug 22, 2023

@Emilgardis

How is this different from CROSS_CONTAINER_OPTS defined here https://github.com/cross-rs/cross/blob/37c681a923407b67044ea09c40205012c0ea86d5/src/docker/shared.rs#L1057C45-L1057C45?

I guess it isn't at all, but the cross README (which is the usual entrypoint for people new to a project) has a section Configuration, which lists three options, two of which link here. Neither of the "highlighted" config options talks about configuration through environment, nor does the cross_toml.md. The file browser visible to the left when reading cross_toml.md through the link listed also shows only a single file in the docs folder and no mention of environment variables.

Finding the docs about environment variables seems to be mostly a matter of coincidence (unless of course I missed something obvious in which case I'm sorry) and now that I found it, it seems to me that this page also supersedes the cross_toml.md linked in the README as there seems to be more information about that file as well. This is also a nice read.

So it seems this is mostly a matter of documentation. I'll happily update the docs if that's alright for you. About exposing the env variable settings through the config file: Should I keep the code snippet you listed and add the file-based config alongside it, or would you prefer if I updated the code in my PR to read CROSS_CONTAINER_OPTS (instead of CROSS_EXTRA_ARGS) and check for the existence of DOCKER_OPTS like the current code does?

@Emilgardis
Copy link
Member

Alright!

Thank you for identifying the cause of the problem.

We link to the wiki in the README under #usage, but this can definitely be improved.

I think we could make the situation you've identified better by

  • Linking to the wiki in docs/cross_toml.md, For additional help and examples, see [link to wiki]
  • Update docs/ to be more like https://github.com/cross-rs/cross/wiki/Configuration, like including the env vars available and better examples, we could probably have CI check that they are in-sync before a release, but we can fix that in another PR.
  • We'd like the README to not be a dumping ground for all config options, but for something like CROSS_CONTAINER_OPTS it would probably be worth mentioning in the readme.

as for the question about config, I'm not entirely sure what I'd prefer myself. There's some alternatives available as I see it

  1. Only have an env var, e.g keep it as-is
  2. Add a new build.container-opts that is another way to set CROSS_CONTAINER_OPTS
  3. Add build.container-opts and target.TARGET.container-opts that is another way to set CROSS_CONTAINER_OPTS

CROSS_DOCKER_OPTS and CROSS_CONTAINER_OPTS should just be aliases of eachother while DOCKER_OPTS is being deprecated.

@har7an
Copy link
Contributor Author

har7an commented Feb 6, 2024

Hi @Emilgardis,

sorry for taking so long but I recently "rediscovered" my previous progress on the matter on my PC and decided to invest some more time into it. I opened a new PR #1438 that deals with the documentation "issue".

I'm fine with leaving the code as is and having only the env var to set additional arguments for the container runtime. This perfectly suits my needs. I'll leave the PR open for now pending a reaction in #1438. If you feel its' no longer needed you can close it as well, of course.

github-merge-queue bot pushed a commit that referenced this pull request Feb 20, 2024
Sort of a follow-up to #1315 which revealed a "mismatch" between what
the `README` and `docs/` folder immediately provide as info compared to
what's actually available.

This restructures the `docs/` folder and adds information from various
sources to it:

- The README,
- The Wiki
- Examples found in the Wiki

At the moment this duplicates a decent amount of information already
available on the Wiki. Strictly speaking this was already the case
before this MR, where the `README` and `cross_toml` each contained
information from the Wiki. My motivation for moving these things into
the `docs/` folder is that:

1. I'm much more used to finding information in a `docs` folder than to
check for the Wiki. Most projects just don't use the Wiki
2. I usually expect the documentation to be part of what I check out,
and the Wiki is *not* a part of this repo, but its own repo
3. Docs can be updated *as part of the same PR* that updates the
respective code, and arbitrary checkouts of the repo will always contain
the docs *valid for that version*

On a personal note, I much prefer editing text in an editor I'm
comfortable with, and Browsers don't fall into that category.

I'm curious to hear your thoughts on this proposed change.
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