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

Install binutils in base docker image #339

Merged
merged 2 commits into from
Jul 20, 2022
Merged

Conversation

Milo123459
Copy link
Collaborator

What does this PR address?

ar has been shown to not be installed in some cases and causes issues. This should fix it.

Before submitting:

  • Make sure to follow GitHub's guide on creating PR.
  • Did you read through contribution guidelines?
  • Did your changes require updates to the documentation? Have you updated those accordingly?
  • Did you write tests to cover your changes? Did it pass locally when running cargo test?
  • Have you run cargo fmt, cargo check and cargo clippy locally to ensure that CI passes?

@coffee-cup
Copy link
Contributor

What is the base image size increase with this? The last I checked it was fairly large.

I think we should prefer to install this only as needed (for specific packages or providers) as opposed to first installing it in the base image. Does this even fix the Rust issues?

@Milo123459
Copy link
Collaborator Author

What is the base image size increase with this? The last I checked it was fairly large.

I think we should prefer to install this only as needed (for specific packages or providers) as opposed to first installing it in the base image. Does this even fix the Rust issues?

I'm not exactly sure how to check the increase.

Though, installing only when needed is extremely hard. We can't just read build scripts and know when a binutils command is being invocated. And, issues with ar not being installed would be fixed by this.

@coffee-cup
Copy link
Contributor

I'm not exactly sure how to check the increase.

Build the base image locally before and after the change. I think it was like a 200MB increase.

Though, installing only when needed is extremely hard. We can't just read build scripts and know when a binutils command is being invocated. And, issues with ar not being installed would be fixed by this.

That is fair. But this has never been an issue with Node, Python, or Ruby projects. So making all of those images 200MB larger isn't ideal. We could just install binutils for Rust projects.

@Milo123459
Copy link
Collaborator Author

Before: 420MB
After: 432MB

I think that change is fine.

@coffee-cup
Copy link
Contributor

I still think we should add it to the Rust provider first before adding it to the base image. I am not opposed to add it to the base in the future. But the default shouldn't be "add to base image". That is how we end up with a massive base that just has everything.

Secondly, does this fix the Rust issues?

@Milo123459
Copy link
Collaborator Author

I'll make this only apply to the rust provider now.

And yes, this fixes the rust issues.

@Milo123459
Copy link
Collaborator Author

Alright, CI is passing and now only binutils is installed in the Rust provider.

@Milo123459 Milo123459 merged commit 5543c4a into main Jul 20, 2022
@Milo123459 Milo123459 deleted the milo/binutils-in-base branch July 20, 2022 22:03
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