pyllyukko / user.js

user.js -- Firefox configuration hardening
MIT License
2.73k stars 233 forks source link

tools: move CI to Github Actions, update documentation, clarify target names, makefile cleanup, add make help target #545

Closed nodiscc closed 11 months ago

nodiscc commented 11 months ago
pyllyukko commented 11 months ago

Thanks for the PR!

I need to do some minor cleanups on this, as you removed some PHONY targets that should remain PHONY and made some non-PHONY targets into PHONY that shouldn't be. Also you seemed to remove some of the automatic variables (namely $@), which I would like to stay as such.

I'll check those through and get this merged soon.

nodiscc commented 11 months ago

you removed some PHONY targets that should remain PHONY

I did not - in fact all targets are now phony.

made some non-PHONY targets into PHONY that shouldn't be

I was careful about this, and concluded that none of the targets should have been PHONY in the first place - which ones do you think should stay phony? For example having 000-tor-browser.js non-phony will prevent newer versions from being downloaded. Also the locked/systemwide/debian...user.js generation targets should be phony, else the output file will not be re-generated if it already exists, even if the source user.js was modified.

pyllyukko commented 11 months ago

Well basically every target that doesn't create a file that is the target name (e.g. diff-tbb) needs to be PHONY. Also vice versa, so if a file called policies.json is to be created, it shouldn't be PHONY. For the issue that new files aren't created it would be better to use the following trick:

.PHONY: FORCE
FORCE:

diff-000-tor-browser.js: FORCE
    ...

Also I'm about to improve the make help target in something like this:

help: # generate list of targets with descriptions
    @sed -n '/^[a-z]/s/^\(.\+\):.*\s\+#\s\+\(.\+\)$$/\1\t\2/p' $(MAKEFILE_LIST) | expand -t20

That way we don't need to have everything as PHONY. If you're cool with that, I'll make the corrections and get forward with the merging?

pyllyukko commented 11 months ago

Something like this https://github.com/pyllyukko/user.js/tree/minor-fixes-to-545?

nodiscc commented 11 months ago

Well basically every target that doesn't create a file that is the target name (e.g. diff-tbb) needs to be PHONY

:+1:

For the issue that new files aren't created it would be better to use the following trick:

I didn't know this trick: https://www.gnu.org/software/make/manual/html_node/Force-Targets.html. Thanks! Not sure about the benefits compared to just using .PHONY though (maybe cleaner/more correct, as .PHONY implies the target does not create a file with the same name?), but it works for me.

improve the make help target in something like this:

Perfect. Tested https://github.com/pyllyukko/user.js/blob/minor-fixes-to-545/Makefile, works fine.

nodiscc commented 11 months ago

Closed accidentally, feel free to reopen.

pyllyukko commented 11 months ago

What...?

nodiscc commented 11 months ago

I'm sorry, my gitea instance is misbehaving. The branch cleanup-tests has been deleted on it, and it is configured to mirror (push) to https://github.com/nodiscc/user.js, so the branch gets deleted again on github/gitlab every time the mirror synchronization runs (every few hours)...

image

I will disable mirroring for now. Sorry for the noise.