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

Enhance Makefiles #51

Closed
wants to merge 3 commits into from
Closed

Conversation

Daniel-Worrall
Copy link
Contributor

@Daniel-Worrall Daniel-Worrall commented Jan 11, 2020

This PR replaces the duplicate references to the targets and prerequisites with magic variables and reworks the Linux Makefile to handle 32 vs 64 bit dynamically where possible. Some targets were rearranged in the interest of keeping similars together.

I did not attempt handling targets with 32/64 in the variable names as this would have required variable changes and quite a bit of conditionals, though it could be handled.

Feedback highly encouraged.

E: Targets tested to be the same through --dry-run

@RX14
Copy link
Contributor

RX14 commented Jan 11, 2020

I'm not a fan of the added complexity just to reduce line count. There's no complex logic which is actually deduplicated here. I'm a fan of the $@ usage change and almost nothing else.

@straight-shoota
Copy link
Member

Also the change introduces weird doc comments. You do realize that double-hash comments on target lines are used for autogenerating usage documentation?

Using $@ is great and reordering to group similar recipies is also good.

The generic %.gz: % and %.xz: % recipies are nice, too. That's actually a simplification.

I think I'm basically fine with everything that doesn't use SECONDEXPANSION.

@RX14
Copy link
Contributor

RX14 commented Jan 11, 2020

If you do reorder, please do it in a seperate commit to the other refactors, otherwise it's difficult to review the diff.

@Daniel-Worrall
Copy link
Contributor Author

Updated, and force pushed.

It should be much easier to review. Took out all the complicated dynamic recipes.

I also tried to format the all64,all32s with dynamic recipes while retaining their comments in the help recipe, but I couldn't come up with a way to process and replace.

.PHONY: all64 all32
all64 all32: all%: compress% package% ## Build compressed and distribution packages for % bits

I also included a change in the tabular formatting to make whitespace more consistent, but it's rather opinionated.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Looks good.

&& docker rm -v "$$container_id"

.PHONY: compress64
compress64: $(OUTPUT_BASENAME64).tar.gz $(OUTPUT_BASENAME64).tar.xz ## Build compressed tarballs

$(OUTPUT_BASENAME64).tar.gz: $(OUTPUT_BASENAME64).tar
gzip -c $(OUTPUT_BASENAME64).tar > $(OUTPUT_BASENAME64).tar.gz
$(OUTPUT_BASENAME64).tar.gz $(OUTPUT_BASENAME32).tar.gz: %.gz: %
Copy link
Member

Choose a reason for hiding this comment

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

Are the first two targets actually necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary, but it restricts it to a static readable list. It can be removed in the interest of brevity but I kept them in in the interest of clarity.

@straight-shoota
Copy link
Member

The first commit of this PR has been merged in #139. The others don't really apply anymore due to the removal of distro package building recipes.
I think this can be closed.

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.

3 participants