Skip to content

Provides explicit makefile targets for files #167

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

Merged
merged 1 commit into from
Jun 25, 2025

Conversation

madrob
Copy link
Contributor

@madrob madrob commented Jun 24, 2025

fixes #165

init depends on vminitd/bin/vminitd, vminitd/bin/vmexec, and bin/cctl
integration depends on bin/containerization-integration
bin/* get generated via make containerization

@madrob madrob requested a review from dcantah June 24, 2025 20:47
Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

I like the simplicity of this. I'm a bit concerned that it's harder to follow/reason about.

@madrob
Copy link
Contributor Author

madrob commented Jun 24, 2025

Are there specific sections that you think are more difficult to understand? We can add some comments there.

I'm not sure what level of familiarity I can expect from folks reading the Makefile, but if the people who expect to maintain it don't understand what's going on then that's probably a good signal that we need more explanation.

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

@madrob I think the changes to make it more clear what binaries are being built helps a lot! Thanks!

@madrob
Copy link
Contributor Author

madrob commented Jun 24, 2025

I think this might have a problem where somebody running make containerization won't get an updated bin/cctl by default now. Will need to think about how to deal with this.

@katiewasnothere
Copy link
Contributor

@madrob Could they not just run all? That reminds me we also probably want to update the CI workflows

@madrob
Copy link
Contributor Author

madrob commented Jun 24, 2025

That reminds me we also probably want to update the CI workflows

What would you update? It looks fine to me, the problem was that I was trying to build stuff and not paying attention to the CI workflows as an example of the correct way to do things.

@katiewasnothere
Copy link
Contributor

@madrob I would change this step make containerization and docs to call make all which includes init, so then we can get rid of building init here

@madrob
Copy link
Contributor Author

madrob commented Jun 25, 2025

I would change this step [make containerization and docs] to call make all which includes init

Do you want all to also make docs? (It does not currently.)

It looks like init currently runs with the 6.2 swift snapshot installed by swiftly, which I think is related to #119 and #94 - I don't want to accidentally regress on that.

@katiewasnothere
Copy link
Contributor

@madrob Fair enough, let's hold off on those changes then.

@madrob
Copy link
Contributor Author

madrob commented Jun 25, 2025

Pushed a squashed commit with signature.

@katiewasnothere katiewasnothere merged commit f9198d6 into apple:main Jun 25, 2025
2 checks passed
dcantah added a commit to dcantah/containerization that referenced this pull request Jun 26, 2025
dcantah added a commit to dcantah/containerization that referenced this pull request Jun 26, 2025
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.

[Request]: Make target dependencies should be made explicit
3 participants