-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Quick fix for resolving issue #11099 #11243
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
Conversation
I think ideally we would still catch errors during generation, for example if a bug in the generation process is introduced or due to other errors. I think it would be possible to detect only ENOEXEC errors. |
@nielsdos, thanks for your suggestion. I define the If presenting the I think it can achieve the recommendation you suggest :). |
Thanks this looks better already. I wonder whether it is possible to store a 1 or 0 into the res variable to clean up the checks. I also think it would be great if a short warning message gets printed when the phar file cannot be generated because of a cross compile, so people will not be surprised it isn't generated. |
@nielsdos, thanks for your advanced recommendations. I use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work! It looks almost good. Just two small mistakes slipped in while you changed the commands: two times you put two commands on the same line instead of keeping them separate.
ext/phar/Makefile.frag
Outdated
@$(INSTALL_DATA) $(builddir)/phar.1 $(INSTALL_ROOT)$(mandir)/man1/$(program_prefix)phar$(program_suffix).1 | ||
@$(INSTALL_DATA) $(builddir)/phar.phar.1 $(INSTALL_ROOT)$(mandir)/man1/$(program_prefix)phar$(program_suffix).phar.1 | ||
@(if [ $(TEST_PHP_EXECUTABLE_RES) -ne 1 ]; then \ | ||
$(mkinstalldirs) $(INSTALL_ROOT)$(bindir) $(INSTALL) $(builddir)/phar.phar $(INSTALL_ROOT)$(bindir)/$(program_prefix)phar$(program_suffix).phar; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You put two separate lines into one line. This used to be two lines:
@$(mkinstalldirs) $(INSTALL_ROOT)$(bindir)
$(INSTALL) $(builddir)/phar.phar $(INSTALL_ROOT)$(bindir)/$(program_prefix)phar$(program_suffix).phar
But now you put them on one line, which means the command won't work properly.
ext/phar/Makefile.frag
Outdated
@$(INSTALL_DATA) $(builddir)/phar.phar.1 $(INSTALL_ROOT)$(mandir)/man1/$(program_prefix)phar$(program_suffix).phar.1 | ||
@(if [ $(TEST_PHP_EXECUTABLE_RES) -ne 1 ]; then \ | ||
$(mkinstalldirs) $(INSTALL_ROOT)$(bindir) $(INSTALL) $(builddir)/phar.phar $(INSTALL_ROOT)$(bindir)/$(program_prefix)phar$(program_suffix).phar; \ | ||
rm -f $(INSTALL_ROOT)$(bindir)/$(program_prefix)phar$(program_suffix) $(LN_S) -f $(program_prefix)phar$(program_suffix).phar $(INSTALL_ROOT)$(bindir)/$(program_prefix)phar$(program_suffix); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem here:
rm -f $(INSTALL_ROOT)$(bindir)/$(program_prefix)phar$(program_suffix)
$(LN_S) -f $(program_prefix)phar$(program_suffix).phar $(INSTALL_ROOT)$(bindir)/$(program_prefix)phar$(program_suffix)
should be on two separate lines (with ; \ like you did for the other lines of course). But you put them on the same line, which makes the command not work properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! This looks good to me and works in my tests. As usual, I'll wait a few days for more feedback. If no more feedback comes in I'll merge it.
Changed log
phar
steps when running the cross compiling.