[FrameworkBundle] improve Makefile serve target #54

sstok · 2017-05-04T08:55:19Z

Related to #49, I don't know if this the best way to do it. But as the Makefile is composed I don't think it's possible to use advanced Makefile features 😟

@greg0ire suggested something like this:

.PHONY: serve

CONSOLE=bin/console

$(CONSOLE):
    @echo 'Woops! The console does not exist'
    @exit

serve: bin/console
    bin/console list server|grep server:start
    bin/console server:start

But don't know if you can have multiple targets with the same name but different conditions?

stof · 2017-05-04T09:02:39Z

symfony/framework-bundle/3.3/Makefile

-	php -S 127.0.0.1:8000 -t web
+	@if [ -a bin/console ] && [[ `bin/console list server | grep server:start` ]]; then \
+		bin/console server:start; \
+		echo -e "Quit the server with \033[32;49mbin/console server:stop.\033[39m"; \


isn't it already displayed by the server:start command itself ?

No, [OK] Server listening on http://127.0.0.1:8000

ah, it is only explained in the command help indeed, not when running the command

btw, if we want to be consistent with the alternate case, which is blocking, this case may need to run server:run instead. Not sure whether consistency is the key here though. What do you think @fabpot ?

greg0ire · 2017-05-04T10:04:10Z

symfony/framework-bundle/3.3/Makefile

 	php -S 127.0.0.1:8000 -t web
+
+serve:
+	@${MAKE} serve_as_sf || ${MAKE} serve_as_php
 .PHONY: serve


You have 2 .PHONY declarations now :P

For serve? Note that multiple .PHONY's is perfectly valid, and this was already used. The Makefiles are combined together so you don't have one absolute header section.

TIL. But you should add serve_as_sf and serve_as_php there too

Done, also found a way to suppress the make error when the command doesn't exist 😄

sstok · 2017-05-04T10:04:16Z

I made a different version with suggestions from @greg0ire 👍

I really need to learn how to fully utilize Makefile.

stof · 2017-05-04T09:18:25Z

symfony/framework-bundle/3.3/Makefile

-	php -S 127.0.0.1:8000 -t web
+	@if [ -a bin/console ] && [[ `bin/console list server | grep server:start` ]]; then \
+		bin/console server:start; \
+		echo -e "Quit the server with \033[32;49mbin/console server:stop.\033[39m"; \


btw, if we want to be consistent with the alternate case, which is blocking, this case may need to run server:run instead. Not sure whether consistency is the key here though. What do you think @fabpot ?

stof · 2017-05-04T10:04:51Z

symfony/framework-bundle/3.3/Makefile

+
+serve_as_sf: $(CONSOLE)
+	@$(CONSOLE)|grep server:start > /dev/null
+	@$(CONSOLE) server:start || exit 0


looks wrong to me. If there is a failure in the command, we want to report it in the main exit code, to let the external system know that it failed

stof · 2017-05-04T10:05:33Z

symfony/framework-bundle/3.3/Makefile

 	@echo "Quit the server with CTRL-C."
-	@echo -e "Run \033[32mcomposer require symfony/web-server-bundle\033[39m for a better web server"
+	@echo "Run \033[32mcomposer require symfony/web-server-bundle\033[39m for a better web server"


removing -e breaks the support of colors AFAIK (you are reverting #48)

No, because when I use this is shows -e Run composer require symfony/web-server-bundle for a better web server

Strange but true.

My colleague @L-P :

use printf, it's more portable, you don't know what echo really is here (shell built-in, make built-in, whatever)

(no flag is required for printf, BTW)

Let's back that up with a very solid source

👍 for printf also, that's really the portable way

greg0ire · 2017-05-04T14:06:30Z

symfony/framework-bundle/3.3/Makefile

-	@echo "Quit the server with CTRL-C."
-	@echo -e "Run \033[32mcomposer require symfony/web-server-bundle\033[39m for a better web server"
+CONSOLE=bin/console
+$(CONSOLE):


maybe : @printf "%s could not be found. You may use an alternate binary by specifying it like this : make CONSOLE=path/to/console some_target" "$(CONSOLE)"

Done. Slightly different.

fabpot · 2017-05-05T16:00:38Z

symfony/framework-bundle/3.3/Makefile

+CONSOLE=bin/console
+$(CONSOLE):
+	@printf "\033[31;49m%s could not be found.\033[39m Run \033[32mcomposer require console\033[39m to install the console.\n" "$(CONSOLE)"
+	@printf "You may use an alternate binary by using: make CONSOLE=path/to/console some_target\n\n"


I would remove this second line as I don't see when one would need this.

fabpot · 2017-05-05T16:22:23Z

I've just tested and it works quite well. Well done!

Here is the output when console and the web server are not installed:

bin/console could not be found. Run composer require console to install the console.
You may use an alternate binary by using: make CONSOLE=path/to/console some_target

/bin/sh: bin/console: No such file or directory
Server listening on http://127.0.0.1:8000
Quit the server with CTRL-C.
Run composer require symfony/web-server-bundle for a better web server
php -S 127.0.0.1:8000 -t web
PHP 7.1.2 Development Server started at Fri May  5 09:17:46 2017
Listening on http://127.0.0.1:8000
Document root is /private/tmp/foobar/web
Press Ctrl-C to quit.

Is it possible to remove the /bin/sh: bin/console: No such file or directory part?

I think we should also use shortcuts when possible:

Run composer req cli to install the Symfony console.
Run composer req webserver for a better web server

fabpot · 2017-05-05T16:23:41Z

Forgot to mention something: the bin/console could not be found. message should probably not be in red as the command works without it; so this is not an error to not have the console installed.

sstok · 2017-05-05T16:56:21Z

I'm away for today, so feel free to push new commits ;)

sstok · 2017-05-06T09:46:57Z

Is it possible to remove the /bin/sh: bin/console: No such file or directory part?

No idea 😐 it's possible suppress errors in a rule but not when the rule itself itself it a target or something. I can make the console a rule but then you still need to check if console exists in each rule, is this acceptable?

CONSOLE=bin/console
sf_console:
	@test -f $(CONSOLE) || printf "Run \033[32mcomposer require cli\033[39m to install the Symfony console.\n"
	@exit

serve_as_sf: sf_console
	@test -f $(CONSOLE) && $(CONSOLE)|grep server:start > /dev/null || ${MAKE} serve_as_php
	@$(CONSOLE) server:start || exit 0

	@printf "Quit the server with \033[32;49mbin/console server:stop.\033[39m\n"

serve_as_php:
	@printf "\033[32;49mServer listening on http://127.0.0.1:8000\033[39m\n";
	@printf "Quit the server with CTRL-C.\n"
	@printf "Run \033[32mcomposer require symfony/web-server-bundle\033[39m for a better web server\n"
	php -S 127.0.0.1:8000 -t web

serve:
	@${MAKE} serve_as_sf
.PHONY: sf_console serve serve_as_sf serve_as_php

$ make serve
Run composer require cli to install the Symfony console.
Server listening on http://127.0.0.1:8000
Quit the server with CTRL-C.
Run composer require symfony/web-server-bundle for a better web server
php -S 127.0.0.1:8000 -t web
PHP 7.1.3 Development Server started at Sat May  6 11:45:52 2017
Listening on http://127.0.0.1:8000

fabpot · 2017-05-11T19:38:36Z

@sstok your proposer changes look good to me.

sstok · 2017-05-12T15:05:13Z

OK, all should be good now.

sstok · 2017-05-12T15:07:11Z

OK, should be good now 👍

fabpot · 2017-05-12T15:13:08Z

Thank you @sstok.

@greg0ire

This PR was merged into the master branch. Discussion ---------- [FrameworkBundle] improve Makefile serve target Related to #49, I don't know if this the best way to do it. But as the Makefile is composed I don't think it's possible to use advanced Makefile features 😟 @greg0ire suggested something like this: ```makefile .PHONY: serve CONSOLE=bin/console $(CONSOLE): @echo 'Woops! The console does not exist' @EXIT serve: bin/console bin/console list server|grep server:start bin/console server:start ``` But don't know if you can have multiple targets with the same name but different conditions? Commits ------- 7a7de4b [FrameworkBundle] improve Makefile serve target

stof · 2017-05-12T16:55:36Z

symfony/framework-bundle/3.3/Makefile

+
+serve_as_sf: sf_console
+	@test -f $(CONSOLE) && $(CONSOLE)|grep server:start > /dev/null || ${MAKE} serve_as_php
+	@$(CONSOLE) server:start || exit 0


exit with 1 when the server fails to start looks wrong to me

@stof

This PR was merged into the master branch. Discussion ---------- fix error exit code In case that the server could not start we should indicate the failure with a non-zero exit code. This addresses @stof's comment in #54 (comment). Commits ------- 9acb162 fix error exit code

@stof

This PR was merged into the master branch. Discussion ---------- fix error exit code In case that the server could not start we should indicate the failure with a non-zero exit code. This addresses @stof's comment in symfony/recipes#54 (comment). Commits ------- 9acb162 fix error exit code

@stof

This PR was merged into the master branch. Discussion ---------- fix error exit code In case that the server could not start we should indicate the failure with a non-zero exit code. This addresses @stof's comment in symfony/recipes#54 (comment). Commits ------- 9acb162 fix error exit code

symfony-bot deployed to staging May 4, 2017 08:55 View deployment

stof reviewed May 4, 2017

View reviewed changes

symfony-bot deployed to staging May 4, 2017 10:03 View deployment

greg0ire suggested changes May 4, 2017

View reviewed changes

stof reviewed May 4, 2017

View reviewed changes

symfony-bot deployed to staging May 4, 2017 10:10 View deployment

sstok force-pushed the 49-framework-bundle-dont-advice-to-install-web-server-bundle-when-already-installed branch from 644b80a to 4c9590e Compare May 4, 2017 10:12

symfony-bot deployed to staging May 4, 2017 10:12 View deployment

sstok force-pushed the 49-framework-bundle-dont-advice-to-install-web-server-bundle-when-already-installed branch from 4c9590e to b8fb6d3 Compare May 4, 2017 13:34

symfony-bot deployed to staging May 4, 2017 13:34 View deployment

greg0ire reviewed May 4, 2017

View reviewed changes

sstok force-pushed the 49-framework-bundle-dont-advice-to-install-web-server-bundle-when-already-installed branch from b8fb6d3 to 50107f4 Compare May 5, 2017 07:27

symfony-bot deployed to staging May 5, 2017 07:27 View deployment

fabpot reviewed May 5, 2017

View reviewed changes

symfony-bot deployed to staging May 8, 2017 22:20 View deployment

[FrameworkBundle] improve Makefile serve target

7a7de4b

sstok force-pushed the 49-framework-bundle-dont-advice-to-install-web-server-bundle-when-already-installed branch from 50107f4 to 7a7de4b Compare May 12, 2017 15:03

symfony-bot deployed to staging May 12, 2017 15:03 View deployment

xabbuh approved these changes May 12, 2017

View reviewed changes

fabpot merged commit 7a7de4b into symfony:master May 12, 2017

stof reviewed May 12, 2017

View reviewed changes

xabbuh mentioned this pull request May 16, 2017

fix error exit code #67

Merged

sstok deleted the 49-framework-bundle-dont-advice-to-install-web-server-bundle-when-already-installed branch August 29, 2017 16:49

Uh oh!

[FrameworkBundle] improve Makefile serve target #54

[FrameworkBundle] improve Makefile serve target #54

Uh oh!

Conversation

sstok commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstok commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstok May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented May 5, 2017

Uh oh!

fabpot commented May 5, 2017

Uh oh!

sstok commented May 5, 2017

Uh oh!

sstok commented May 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabpot commented May 11, 2017

Uh oh!

sstok commented May 12, 2017

Uh oh!

sstok commented May 12, 2017

Uh oh!

fabpot commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greg0ire May 4, 2017 •

edited

Loading

greg0ire May 4, 2017 •

edited

Loading

sstok May 5, 2017 •

edited

Loading

sstok commented May 6, 2017 •

edited

Loading