Skip to content
This repository has been archived by the owner on Feb 3, 2019. It is now read-only.

isolate creation of cross compiler with docker #6

Merged
merged 4 commits into from Dec 11, 2015

Conversation

schnupperboy
Copy link

This pull request contains the following changes and thus closes #5:

  • implemented a Dockerfile which is able to build a Docker image which contains the cross compiler (Rust version can specified)
  • documented motivation and usage of building the Docker image and cross compiling a rust project by running a Docker container from that previously built Docker image
  • split the initial README.md into MANUAL.md and DOCKER.md

@schnupperboy schnupperboy mentioned this pull request Dec 9, 2015
@Ogeon
Copy link
Owner

Ogeon commented Dec 9, 2015

Nice! I have just mostly skimmed through it (I'll look closer later), but I would like to comment on a few things straight away:

  • It looks like the formating of the list is incorrect (press "View" in "File changes"), unless it should be baked into the paragraph.
  • Could you mention DOCKER.md in README.md and maybe even link to it? That would make it easier to discover.
  • It's not obvious what the dependency directory is, so it would be nice if it was further explained. Also, would it be possible to make it optional? I think that the more we can slim it down, the better.
  • It's also not obvious that the docker directory in cd docker refers to the directory in this repo. Could you include a git clone command or mention that it should be downloaded from here? It's just for the sake of clarity.

It looks good otherwise, as far as I can tell at the moment.

@schnupperboy
Copy link
Author

Thanks for your feedback. All of your points should be fixed now. I also put the documentation of the manual process into a seperate markdown file just like DOCKER.md and linked to both of them from README.md.

@@ -0,0 +1,63 @@
# Cross compiling with `Docker`
The manual process kind of pollutes your workstation by setting lots of environment variables and writing to various config files. Thus it can be difficult to remove or upgrade these files and settings or even repeat that process for different versions of rust on the same machine.
Copy link
Owner

Choose a reason for hiding this comment

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

This is a bit exaggerated, to be honest. The "unclean" part would the gcc-sysroot script + the config files. The cross scripts are meant to take care of and contain the environment variables, to leave the system untouched. So, the parts about various config files and difficulty to remove, upgrade and change version are at least correct.

What I'm trying to say is that there is no real reason to paint the manual method as worse than it is.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I read it again and I know what you mean. It was not my intention to comment on the specific implemented process. Just changed this part of the manual accordingly in the latest commit.

Copy link
Owner

Choose a reason for hiding this comment

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

It's alright. That's why it's nice to have someone reviewing the changes 😄 Both methods are good for different reasons, as you wrote in the readme, which is why I prefer to keep things somewhat neutral.

@Ogeon
Copy link
Owner

Ogeon commented Dec 10, 2015

I have taken a closer look and the changes are very good. There is only the concern about the motivation for using Docker, which is a small thing. The mere simplicity of the process is already a big motivator, so that paragraph can be toned down a bit.

The Docker file and the additional scripts are straight forward and looks good. I'm ready to merge this after the motivation thing has been sorted out. It would also be nice if you could edit the description of this pull request to summarize the changes and refer to #5. Preferrably as "Closes #5".

@Ogeon
Copy link
Owner

Ogeon commented Dec 10, 2015

The new motivation is much better. I like it. Now it's just the PR description, and we are good to go.

@schnupperboy schnupperboy changed the title isolate creation of cross compiler with docker Closes #5 Dec 10, 2015
@Ogeon
Copy link
Owner

Ogeon commented Dec 11, 2015

What I meant was to edit the description (or initial comment) and not the title. I think it's nice to have a short summary at the top of the thread, in case one would like to go back to it again. Nothing fancy, just so it's easy to get an overview. The previous title was alright as it was.

@schnupperboy schnupperboy changed the title Closes #5 isolate creation of cross compiler with docker Dec 11, 2015
@schnupperboy
Copy link
Author

Ah, I got it now :)

Ogeon added a commit that referenced this pull request Dec 11, 2015
Isolate creation of cross compiler with docker
@Ogeon Ogeon merged commit 21b65e6 into Ogeon:master Dec 11, 2015
@Ogeon
Copy link
Owner

Ogeon commented Dec 11, 2015

Very nice! Thank you so much for the help!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dockerize the process
2 participants