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

Implemented custom images #44

Merged
merged 2 commits into from
Jan 8, 2017
Merged

Conversation

Emilgardis
Copy link
Member

@Emilgardis Emilgardis commented Jan 6, 2017

See issue #42

These changes follow option 1 on the issue and has
support for expansion as such: --docker-image {target}.
EDIT:: Reworked to work with Cross.toml instead

@@ -220,6 +220,10 @@ impl From<Host> for Target {
}
}

pub type Tag = Option<String>;

pub struct DockerImage(Option<String>);
Copy link
Member Author

Choose a reason for hiding this comment

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

I should probably just make this into a newtype like I did with Tag.

@japaric
Copy link
Contributor

japaric commented Jan 7, 2017

Thanks for the PR, @Emilgardis.

I rather not extend the CLI; I want to keep it exactly the same as Cargo. Could you move the image overrides into a Cross.toml file that sits next to Cargo.toml? That file would apply to all the cross calls inside the project and that way you don't have to pass e.g. --docker-image to each cross call.

I'm thinking of something like this:

# Cross.toml
[target.armv7-unknown-linux-gnueabihf]
image = "user/repo:tag"  # or "user/repo" (implies :latest) or just "repo" (doesn't look up the image in DockerHub)

@Emilgardis
Copy link
Member Author

Emilgardis commented Jan 7, 2017

Fair enough. I have however a question about your example here.

just "repo" (doesn't look up the image in DockerHub)

Does that mean cross should parse image and see if it is of form user/repo or just repo.
(Or is it ok to actually let docker do the checking, i.e docker tries to use -it repo)

@japaric
Copy link
Contributor

japaric commented Jan 7, 2017

Just passing the image string as it is to Docker is fine. Docker will print an error if it can't find it.

@Emilgardis
Copy link
Member Author

Ok, I'll remove the check on local images aswell, I feel like it is uneeded with this change.

@Emilgardis Emilgardis force-pushed the issue-42 branch 2 times, most recently from 5c652ab to 26970a2 Compare January 7, 2017 17:26
Use a Cross.toml file located at manifest dir.

* `target.{target_tripe}.image`
  to set image used by docker
* `target.{target_triple}.tag`
  cannot be used with image (ignored) but will set the tag
  for image `japaric/{target_triple}`

Closes cross-rs#42
@Emilgardis
Copy link
Member Author

I've implemented the images now using a Cross.toml file.

@Emilgardis Emilgardis changed the title Add --docker-tag and --docker-image, also make cross smarter Implemented custom images Jan 7, 2017
@japaric
Copy link
Contributor

japaric commented Jan 8, 2017

Thanks for updating the PR, @Emilgardis.

I'd rather not expose an easy way to use a different tag of the japaric/$TARGET image as e.g. tag v0.1.2 is not guaranteed to work with cross v0.1.1 and viceversa. And I definitely don't want an easy way for someone to use :latest with an stable cross release. No end user should ever use :latest as it is a moving target (it changes everytime a PR lands in master).

If you remove target.$target.tag, I'll r+.

@japaric
Copy link
Contributor

japaric commented Jan 8, 2017

Thanks again, @Emilgardis!

@homunkulus r+

@homunkulus
Copy link
Contributor

📌 Commit a32f4f0 has been approved by japaric

@homunkulus
Copy link
Contributor

⌛ Testing commit a32f4f0 with merge 938efc9...

japaric pushed a commit that referenced this pull request Jan 8, 2017
Implemented custom images

See issue #42

~~These changes follow option 1 on the issue and has~~
~~support for expansion as such: `--docker-image {target}`.~~
**EDIT::** Reworked to work with Cross.toml instead
@homunkulus
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: japaric
Pushing 938efc9 to master...

@homunkulus homunkulus merged commit a32f4f0 into cross-rs:master Jan 8, 2017
@Emilgardis Emilgardis deleted the issue-42 branch April 6, 2022 09:18
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.

None yet

3 participants