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

fix: Makefile syntax and whitespace quoting #207

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

hyperupcall
Copy link
Contributor

@hyperupcall hyperupcall commented Nov 18, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

hyperupcall commented Nov 20, 2023

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
Copy link
Collaborator

rpdelaney commented Nov 20, 2023

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
Copy link
Collaborator

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

@hyperupcall
Copy link
Contributor Author

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 rpdelaney merged commit eadc123 into trapd00r:master Dec 12, 2023
2 checks passed
@rpdelaney
Copy link
Collaborator

Thank you! 💯

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.

Invalid syntax in Makefile?
2 participants