fix: Makefile syntax and whitespace quoting

trapd00r / LS_COLORS

A collection of LS_COLORS definitions; needs your contribution!

Other

2.11k stars 260 forks source link

fix: Makefile syntax and whitespace quoting #207

Closed hyperupcall closed 10 months ago

hyperupcall commented 11 months ago

As mentioned in the linked issue, the previous syntax caused a syntax error if XDG_DATA_HOME was undefined.

$ unset XDG_DATA_HOME
$ make install
/bin/sh: ${$HOME/.local/share}: bad substitution
make: *** [install] Error 1

This does away with the $$ escaping, and leans into Make's standard $() substitution syntax. The logic goes like this:

If XDG_DATA_HOME is not defined, assign it to $HOME/.local/share (now, value of $HOME is evaluated right away instead of being deferred; effectively changes nothing)
Quote $(XDG_DATA_HOME), in case parts of it contain whitespace (Make does not evaluate the quotation marks, Bash will once $(XDG_DATA_HOME) is substituted)

This also removes the export - it's not necessary

Closes #201

rpdelaney commented 11 months ago

Thanks so much for these PRs~! <3

I have such deep experience with GNU bash and so little experience with GNU make that when I look at this stuff my brain tells me Make is Bash with sugar, but it's not. I need to spend a little time going over the GNU make manual to grok this better.

One problem I'm chewing on is how we can add regression tests in CI for these things while we're at it. If you have any thoughts on that please let me know!

hyperupcall commented 11 months ago

I have such deep experience with GNU bash and so little experience with GNU make that when I look at this stuff my brain tells me Make is Bash with sugar, but it's not. I need to spend a little time going over the GNU make manual to grok this better.

Haha I know the feeling - I still have to manually test my changes for Makefiles since it can be seemingly unpredictable sometimes.

One problem I'm chewing on is how we can add regression tests in CI for these things while we're at it. If you have any thoughts on that please let me know!

Maybe I can add a .github/workflows/ci.yaml action that runs on [pull_request] and commits to master, running scripts in ~/tests (via run_tests). And add a test for checking that the XDG_ variable doesn't need to be defined for the script to succeed. I can do that in a separate PR

rpdelaney commented 11 months ago

I can do that in a separate PR

That would be great since it would demonstrate that the test fails when it should. Test driven design! :) My plate is full today but I should be able to take a swing at it later this week; no hard feelings if you beat me to it.

Edit: Let's call it .github/workflows/tests.yaml since it's more descriptive and there are already a couple workflows in this repo.

rpdelaney commented 10 months ago

@hyperupcall Can you merge from master? It looks like collaborator edits wasn't checked

hyperupcall commented 10 months ago

Done! Unfortunately, GitHub does not seem to show show a collaborator edits checkbox when a fork is not from a personal account.

It looks like the tests pass :)

rpdelaney commented 10 months ago

Thank you! 💯