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

Makefile improvements #371

Merged
merged 2 commits into from Dec 29, 2017

Conversation

greg0ire
Copy link
Contributor

@greg0ire greg0ire commented Dec 9, 2017

No description provided.

We are less likely to forget it next time
@greg0ire
Copy link
Contributor Author

greg0ire commented Dec 9, 2017

Proof PR: sonata-project/SonataAdminBundle#4829

@jordisala1991
Copy link
Member

Can you explain how we use this makefile? I have no idea what .PHONY is.

Not sure if we should have some docs related on the usage of this makefile for the contributors

@greg0ire
Copy link
Contributor Author

. PHONY means this is not a file, don't bother checking the modification date

@jordisala1991
Copy link
Member

jordisala1991 commented Dec 10, 2017

So the commands are:

Those are only for check if there are issues (used by travis)
make lint -> lint everything
make lint-yaml -> lint yaml
make lint-composer -> composer validate
make lint-xml -> lint xml
make lint-php -> php-cs-fixer

Those are for fixing the issues (used by the people)
make cs-fix
make cs-fix-php
make cs-fix-xml

make test
make docs

did I get it right?

@jordisala1991
Copy link
Member

This line is no longer needed and can be removed: https://github.com/sonata-project/dev-kit/blob/master/project/.travis.yml.twig#L45

@greg0ire
Copy link
Contributor Author

AFK, but yeah you got it right!

@soullivaneuh
Copy link
Member

I really don't think we need a .PHONY for each task. You don't have any file or directory named lint-yaml on our project AFAIK.

@greg0ire
Copy link
Contributor Author

greg0ire commented Dec 14, 2017

That's why you don't need to access the disk to check for the file. And it is required for the docs target, at least, because there is a docs directory. Also, what if a lint-yaml directory is created in the future?

@soullivaneuh
Copy link
Member

soullivaneuh commented Dec 14, 2017

That's why you don't need to access the disk to check for the file.

Could you please elaborate?

And it is required for the docs target, at least, because there is a docs directory.

Yeah, this is why it's already specified on current master.

@greg0ire
Copy link
Contributor Author

Could you please elaborate?

This is best understood when running make in debug mode. It does way less checks when there are .PHONY, because of implicit rules it has: https://ftp.gnu.org/old-gnu/Manuals/make-3.79.1/html_chapter/make_10.html#SEC97 I don't remember how to reproduce this but that's something I experienced. Also, it's a good practice to always put .PHONY when you should because it might cause bugs if you don't notice there is a file / directory named like that, just like in docs case.

@greg0ire
Copy link
Contributor Author

More details about the performance implications here: https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html

@soullivaneuh soullivaneuh merged commit 9119037 into sonata-project:master Dec 29, 2017
@soullivaneuh
Copy link
Member

Thank you @greg0ire

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