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

Add Makefile #41

Merged
merged 7 commits into from
Oct 5, 2020
Merged

Add Makefile #41

merged 7 commits into from
Oct 5, 2020

Conversation

SuperSandro2000
Copy link
Contributor

No description provided.

@lyknode
Copy link
Contributor

lyknode commented Oct 1, 2020

Hi @SuperSandro2000,

I was going to take care of that but you did it first ^^. Allow me to comment on some suggestions.

  • Could you include support for $(DESTDIR)? Those are used by packaging tools, allowing files to be installed in a relative path from PREFIX.
    Usually in makefiles, you would find:
PREFIX ?= /usr/local
mandir := $(DESTDIR)$(PREFIX)/man
man1dir := $(mandir)/man1
bindir := $(DESTDIR)$(PREFIX)/bin
  • Could you not include test and install in the all target? By convention, make must only build and not require privileges (which install might depending on DESTDIR and PREFIX), ideally users do make && make test && sudo make install.
  • Could you explicitly install mgitstatus with a 755 mode?
  • Could you not add --no-print-directory, this is GNU make specific and users and tools might want/expect path to be printed.

The next two suggestions are just my preference so fell free to ignore those totally:

  • You are not using any syntax specific to bash, so you could replace SHELL by /bin/sh to allow building on a wider set of systems.
  • You don't need DEFAULT_GOAL if all is your first target in the makefile, which it is by convention.

@lyknode
Copy link
Contributor

lyknode commented Oct 1, 2020

Also, you didn't remove build.sla and install.sh.

@SuperSandro2000
Copy link
Contributor Author

Could you not add --no-print-directory, this is GNU make specific and users and tools might want/expect path to be printed.
You are not using any syntax specific to bash, so you could replace SHELL by /bin/sh to allow building on a wider set of systems.

I just copied that from my default Makefile.

Changed all the things you requested.

@lyknode
Copy link
Contributor

lyknode commented Oct 1, 2020

Thanks for the quick fix!

You are missing a .PHONY tag for the install target and then it should be good.

@SuperSandro2000
Copy link
Contributor Author

@lyknode Added that

@lyknode
Copy link
Contributor

lyknode commented Oct 2, 2020

Ah, we forgot the clean rule. Could you make it remove the generated manpage?

@SuperSandro2000
Copy link
Contributor Author

The manpage is currently tracked in git and removing it would cause changes to the cwd.

When the manpage is converted to troff we don't need the generation or clean.

@lyknode
Copy link
Contributor

lyknode commented Oct 2, 2020

As per last comment on #38, the manpage would be removed and the markdown kept. But you are right, the clean target doesn't need to be added right now. It can be done afterward as the file is removed from the repo.

@fboender fboender merged commit 7320b89 into fboender:master Oct 5, 2020
@SuperSandro2000 SuperSandro2000 deleted the makefile branch October 5, 2020 20:04
@lyknode lyknode mentioned this pull request Apr 3, 2022
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.

None yet

3 participants