Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Allow 'make' to work first time #229

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

Closed
fredden wants to merge 1 commit into phpstan:1.5.x from fredden:makefile-quickstart
Closed

Allow 'make' to work first time #229

fredden wants to merge 1 commit into phpstan:1.5.x from fredden:makefile-quickstart

Conversation

@fredden
Copy link

@fredden fredden commented Oct 6, 2023

I am looking into a suspected bug, so I cloned this repository to review the code. I wanted to check that the tests worked locally on the main branch before trying to make any changes. However, when I ran make, I got errors. The changes in this pull request allow make to work first time.

Copy link
Member

Hi, I'd need to understand these advanced Makefile features and I'm not sure if I want to 😊

Copy link
Author

fredden commented Oct 6, 2023

Putting things after the rule lists their dependencies. A dependency can either be another target, or a file. You're already using this for the 'check' line:

check: lint cs tests phpstan

The changes here add two new targets, and list them as dependencies where relevant.

  • vendor - when changes detected in 'composer.json' or 'composer.lock' files, then the commands within that target are run: composer install
  • composer.lock - because this file doesn't exist in the repository (see .gitignore), make will fail the vendor target without being told how to make this file be created.

Running the cs target before running cs-install results in an error (because the files don't exist). The same is the case for lint or tests where the commands in vendor/bin/ do not yet exist.

Copy link
Member

Thank you, but:

  1. What the composer.json target does? vendor: composer.json composer.lock
  2. I don't want to run cs-install every time when I run make cs
  3. Is it a coincidence or on purpose that vendor target has the name of the vendor directory?
  4. I think that generally make can work on Windows, but I'm pretty sure this syntax breaks that: test -f composer.lock
  5. I'd want for make to work the same across all repositories in https://github.com/phpstan and I'm not sure if you're going to update all of them. These updates are also not the priority for me right now so I don't know if I'm going to review them and merge them in timely manner.
  6. The flow of vendor isn't ideal in some scenarios - for example for the first run it's going to run composer update and composer install right afterwards.

Because of these many issues and questions and also because I'm not a fan of complexity in the Makefile, I decided not to merge this PR. I work on these projects daily and in my opinion the current experience is okay.

Copy link
Author

fredden commented Oct 6, 2023

Thanks for taking the time to consider this and for explaining your perspective. I can address all of these issues/questions, but don't want to burden you any further.

@fredden fredden deleted the makefile-quickstart branch October 6, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /