Makefile improvements #195

greg0ire · 2020-06-03T17:21:32Z

No description provided.

greg0ire · 2020-06-03T17:22:15Z

Makefile

@@ -18,9 +18,10 @@ test-fix: vendor
 update-compatibility-patch:
 	@git apply tests/php-compatibility.patch
 	@echo -e "Please open your editor and apply your changes\n"
-	@until [ "$${compatibility_resolved}" == "y" ]; do read -p "Have finished your changes (y|n)? " compatibility_resolved; done && compatibility_resolved=
+	@$$EDITOR tests/expected_report.txt


Can somebody that uses Mac OS test this please?

What would you like me to run? just make update-compatibility-patch?

-e Please open your editor and apply your changes /bin/sh: tests/expected_report.txt: Permission denied make: *** [update-compatibility-patch] Error 126

Looks like your version of echo doesn't support -e. Also, looks like you don't have a $EDITOR variable set, can you confirm that?

Well yeah I confirm that. Setup should be made to deal with that though, right?

Setup should be made to deal with that though, right?

Can you elaborate? I don't understand what you mean.

I mean Makefile should handle the case when $EDITOR is not defined

Ah is see, with EDITOR ?= vim for instance?

Seems a bit forceful, I will just drop that commit.

greg0ire · 2020-06-03T17:40:10Z

Oh wow the build is so broken… seems to have to do with Composer. Looks like we are already using v2 for some reason.

greg0ire · 2020-06-03T19:50:09Z

I cherry-picked the last commit in #196 , since it is far more urgent.

ostrolucky · 2020-06-04T10:40:59Z

Makefile

@@ -1,6 +1,6 @@
 .PHONY: test test-report test-fix update-compatibility-patch

-PHP_74_OR_NEWER=`php -r "echo (int) version_compare(PHP_VERSION, '7.4', '>=');"`
+PHP_74_OR_NEWER=$(shell php -r "echo (int) version_compare(PHP_VERSION, '7.4', '>=');")


why this change?

Without this change, the command is run every time the variable is evaluated.

Interesting, is that Makefile specific thing? Because this shouldn't be the case in bash. And is same justification used for both changes: changing `` into $() AND adding `shell`? Where can I find something more about this?

Yes, it's very much Makefile specific. It looks like an alternative would be to use := note that using backticks is deprecated in favor of $() in bash.
Also note that $(shell foo) is a function call and does not result in make calling an hypothetical shell binary. I think I am going to go with :=, (or both that and $(shell)? I have yet to figure out whether to stick with backticks or not).

Ok I'm going to go with both: https://stackoverflow.com/questions/1435861/computing-makefile-variable-on-assignment, but the most important is the $(shell ) here.

ostrolucky · 2020-06-04T17:09:08Z

Makefile

 	@git diff -- tests/expected_report.txt tests/fixed > .tmp-patch && mv .tmp-patch tests/php-compatibility.patch && git apply -R tests/php-compatibility.patch
 	@git commit -m 'Update compatibility patch' tests/php-compatibility.patch

 vendor: composer.json
 	composer update
+	touch vendor


What about this, why are we assuming composer update won't create vendor folder?

We are not assuming it won't create the vendor folder, but we are assuming (rightfully) that it will not change its modification date if it already exists. (touch is used to create files, not directories, and bumps the modification date when used on either)

So this updates timestamp of vendor folder, which will make sure dependant targets like test-report will run even when vendor wasn't changed 👍

If we only want to modify the date, should we use touch -c instead? Additionally, are we interested in the directory itself or in all its files (something like $(shell find vendor -type f))? That way, we wouldn't have to touch the directory.

If we only want to modify the date, should we use touch -c instead?

@ostrolucky or you @morozov if you use Mac OS can you test touch -c or better, touch --no-create with sh to see if it is portable? I have no idea.

Additionally, are we interested in the directory itself or in all its files (something like $(shell find vendor -type f))? That way, we wouldn't have to touch the directory.

What are you suggesting here? make only checks the vendor directory's timestamp (because it is the target). It will not check any of the underlying files.

Ar you suggesting we do

$(shell find vendor -type f)): composer.json composer update

If yes, how would you even invoke such a target?

@morozov if you use Mac OS

That sounds insulting. No.

What are you suggesting here?

I meant it to be used in the dependency context e.g. target: foo bar $(shell find vendor -type f)). But I'm not really sure how it's used here, so nevermind.

The contents of the vendor directory only depends on 2 files composer.json and composer.lock. And by depends, make means they are read (by Composer in this case) when generating that directory.

@ostrolucky ran a quick test --no-create is not supported, but -c is, let's go with that.

Make relies on modification dates to decide whether to rebuild a target or to skip it. Running make vendor twice now gives this message the second time: > make: 'vendor' is up to date. And of course, it is much faster.

This should prevent upgrading to Composer v2 by mistake

Looks like echo -e does not work properly with MacOs's sh

greg0ire · 2020-06-25T10:46:47Z

@lcobucci why did you assign this to yourself? Do you mean to test it locally before merging, maybe?

lcobucci · 2020-06-25T11:21:22Z

I was going to test it locally and merge it but then life happened.

greg0ire · 2020-08-19T18:29:24Z

ping @lcobucci 🙏

lcobucci · 2020-08-20T12:57:03Z

@greg0ire sorry, mate! 🚢

greg0ire commented Jun 3, 2020

View reviewed changes

greg0ire force-pushed the makefile-improvements branch from 2072f5a to 294364e Compare June 3, 2020 17:40

This comment has been minimized.

Sign in to view

greg0ire requested a review from a team June 3, 2020 19:50

ostrolucky reviewed Jun 4, 2020

View reviewed changes

Evaluate shell command once

86eb322

greg0ire force-pushed the makefile-improvements branch from d23d995 to 88a2bbd Compare June 4, 2020 17:00

ostrolucky reviewed Jun 4, 2020

View reviewed changes

greg0ire force-pushed the makefile-improvements branch from 88a2bbd to 7b124b6 Compare June 4, 2020 17:15

ostrolucky previously approved these changes Jun 4, 2020

View reviewed changes

SenseException previously approved these changes Jun 4, 2020

View reviewed changes

greg0ire added 3 commits June 4, 2020 21:44

Bump the vendor directory modification date

334e1d9

Make relies on modification dates to decide whether to rebuild a target or to skip it. Running make vendor twice now gives this message the second time: > make: 'vendor' is up to date. And of course, it is much faster.

Update to v2 of the action

bb539af

This should prevent upgrading to Composer v2 by mistake

Use a more portable way to add a carriage return

f66a686

Looks like echo -e does not work properly with MacOs's sh

greg0ire dismissed stale reviews from SenseException and ostrolucky via f66a686 June 4, 2020 19:45

greg0ire force-pushed the makefile-improvements branch from bc3659a to f66a686 Compare June 4, 2020 19:45

ostrolucky approved these changes Jun 4, 2020

View reviewed changes

SenseException approved these changes Jun 6, 2020

View reviewed changes

greg0ire requested review from morozov and a team June 20, 2020 10:36

morozov approved these changes Jun 22, 2020

View reviewed changes

beberlei approved these changes Jun 22, 2020

View reviewed changes

lcobucci added this to the 9.0.0 milestone Jun 22, 2020

lcobucci self-assigned this Jun 22, 2020

lcobucci approved these changes Jun 22, 2020

View reviewed changes

lcobucci merged commit 98aa0e9 into doctrine:master Aug 20, 2020

greg0ire deleted the makefile-improvements branch August 20, 2020 17:24

greg0ire modified the milestones: 9.0.0, 8.2.0 Oct 25, 2020

github-actions bot mentioned this pull request Oct 25, 2020

Merge release 8.2.0 into 8.3.x #231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile improvements #195

Makefile improvements #195

greg0ire commented Jun 3, 2020

greg0ire Jun 3, 2020

ostrolucky Jun 4, 2020

greg0ire Jun 4, 2020

ostrolucky Jun 4, 2020

greg0ire Jun 4, 2020

ostrolucky Jun 4, 2020

greg0ire Jun 4, 2020

greg0ire Jun 4, 2020

greg0ire commented Jun 3, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

greg0ire commented Jun 3, 2020

ostrolucky Jun 4, 2020

greg0ire Jun 4, 2020

ostrolucky Jun 4, 2020

greg0ire Jun 4, 2020 •

edited

Loading

greg0ire Jun 4, 2020 •

edited

Loading

ostrolucky Jun 4, 2020

greg0ire Jun 4, 2020 •

edited

Loading

ostrolucky Jun 4, 2020

morozov Jun 4, 2020

greg0ire Jun 4, 2020 •

edited

Loading

morozov Jun 4, 2020

greg0ire Jun 4, 2020

greg0ire Jun 4, 2020

greg0ire commented Jun 25, 2020

lcobucci commented Jun 25, 2020

greg0ire commented Aug 19, 2020

lcobucci commented Aug 20, 2020

Makefile improvements #195

Makefile improvements #195

Conversation

greg0ire commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Jun 3, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

greg0ire commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

greg0ire Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Jun 25, 2020

lcobucci commented Jun 25, 2020

greg0ire commented Aug 19, 2020

lcobucci commented Aug 20, 2020

greg0ire commented Jun 3, 2020 •

edited

Loading

greg0ire Jun 4, 2020 •

edited

Loading

greg0ire Jun 4, 2020 •

edited

Loading

greg0ire Jun 4, 2020 •

edited

Loading

greg0ire Jun 4, 2020 •

edited

Loading