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

Improve makefile #577

Merged
merged 4 commits into from
Sep 20, 2022
Merged

Conversation

brauliohms
Copy link
Contributor

Follows improve Makefile for better compatibility with other systems. Patch according to Debian packaging, because I am the maintainer of the package in Debian.

@Euro20179
Copy link
Collaborator

The install rule should not also run the doc and addons rules, they should remain separate.
Some people may not want documentation, and the addons are meant to be addons, not something that comes with the program unless explicitly opted into.

Also the all rule does nothing.

@Euro20179 Euro20179 closed this Sep 19, 2022
@brauliohms
Copy link
Contributor Author

brauliohms commented Sep 20, 2022

The current makefile not containing the 'ALL' will have a problem when doing the make command, because make is equal to the make all command and the fact of not having the all: blank, will perform the installation of the first target that is the doc: then the recommended would be to put the all: without dependencies and commands for the make command not to install the doc target.

@Euro20179
Copy link
Collaborator

If doing just make runs the first target, then I think I can just move install to the top.
When I add the all: target, and run make it says Nothing to be done for 'all' even though ytfzf is not installed.

@brauliohms
Copy link
Contributor Author

If you put just the all: it is enough for the make command to do nothing in the case of YTFZF, because the make command is for building and in the case of ytfzf we don't need to build, we just need to install. So a good suggestion to improve the Makefile, would be to add the blank all:.

@Euro20179
Copy link
Collaborator

would it be a bad idea to do all: install instead of having a blank all:?

@brauliohms
Copy link
Contributor Author

if you put all: install the make command will run the 'install target', and this is not correct, because the all: is used to build a program (if it was in C it would be time to use GCC to build the binary) but in the case of YTFZF the binary is a script and does not need to be built, just installed, so you need to leave a blank build instruction with all: for nothing to be built. When you need to install the binary (ytfzf script) the correct is to use make install or make install docs addons (according to your need).

@Euro20179
Copy link
Collaborator

Ok, cool, I'll reopen this pull request, but can you get rid of the install: doc addons part?

@Euro20179 Euro20179 reopened this Sep 20, 2022
@brauliohms
Copy link
Contributor Author

Ok, I understood that the user has the freedom of what to install, I will remove this part of the patch.

@Euro20179 Euro20179 merged commit 0988590 into pystardust:development Sep 20, 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

2 participants