Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

propel-gen.bat different when installing Propel via Composer #588

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Member

staabm commented Mar 3, 2013

Just moved away from a PEAR setup propel project and went to a composer based approach.

After installing propel1 using composer I recognized, that the propel-gen.bat contains:

@ECHO OFF
SET BIN_TARGET=%~dp0\"../propel/propel1/generator/bin"\propel-gen
bash "%BIN_TARGET%" %*

when running vendor\bin\propel-gen.bat it aborts with a bash not found error.

Member

staabm commented Feb 1, 2013

changing the batch file to just

@ECHO OFF
%~dp0\..\propel\propel1\generator\bin\propel-gen

and it works as expected...

Don't know where this file is defined within propel (or composer)?

Owner

willdurand commented Feb 1, 2013

PR?

William Durand | http://www.williamdurand.fr

On Fri, Feb 1, 2013 at 5:54 PM, Markus Staab notifications@github.comwrote:

changing the batch file to just

@echo OFF
%~dp0..\propel\propel1\generator\bin\propel-gen

and it works as expected...


Reply to this email directly or view it on GitHubhttps://github.com/propelorm/Propel/issues/588#issuecomment-13002781.

Member

staabm commented Feb 1, 2013

Sure, but I don't know where this batch file is located.. It is not the one located in generator/lib/propel-gen.bat

Member

staabm commented Feb 1, 2013

Hmm I suppose the errornous file is composer generated....
composer/composer@7e3f809#commitcomment-2545999

Contributor

Seldaek commented Feb 1, 2013

@staabm I don't see why that change should affect the end result. If propel-gen has a shebang that calls bash, calling it with bash directly should work just the same. If it says bash not found it shouldn't find it via the shebang either. The reason we do that in composer is that on windows if you don't use bash/gitshell, the shebang isn't read, so we read it and add that process as called in the .bat file.

Member

staabm commented Feb 1, 2013

@Seldaek so I cant use the batch from the windows command line, because there is no bash command? Or do I need to install some tooling to be able to use the batch files?

Contributor

Seldaek commented Feb 1, 2013

Well, yeah, if bash is not in your PATH it's not gonna work from cmd.
It's not composer's fault if the propel script uses bash instead of php.
Bash is not as portable unfortunately.

Member

staabm commented Feb 1, 2013

I think by default bash will not be on PATH on every windows system?

Hmm propel provides a bash and also a batch file for the cli, maybe this is something composer cannot cope with?
Or in other words: does composer generate this vendor/bin/propel-gen file containing the bash command because propel also provides a propel-gen.sh?

Contributor

Seldaek commented Feb 1, 2013

Traditionally many programs came with both bash and batch files that were just starting a php file. Composer makes it easy to just have one php file marked executable, and if you put a shebang in it, it'll work fine on nix and windows. I think that's easier, and I would encourage everyone to migrate to this solution. If it's not practical then maybe listing both batch and bash files in composer bin would help, but not sure about this.

Member

staabm commented Feb 2, 2013

Don't know how to solve this. I have no clue how and why this file is generated in the way it is by composer.
In Propels' repos there is no batch file which contains a bash command.

Member

staabm commented Feb 4, 2013

Just tried how it works when the "bash" command is available at CLI.

The propel-gen.bat runs in another error, since it cannot find phing command

> vendor\bin\propel-gen
# output..
\vendor\bin\\../propel/
propel1/generator/bin\propel-gen: line 50: phing: command not found
Contributor

Seldaek commented Feb 11, 2013

The problem is that https://github.com/propelorm/Propel/blob/master/generator/bin/propel-gen#L1 is using bash as executable, it is a bash script. And since it's listed first in the composer.json, composer creates a windows proxy for it (propel-gen.bat), then the original propel-gen.bat (https://github.com/propelorm/Propel/blob/master/generator/bin/propel-gen.bat) comes and it's not created since it already got created as the proxy to the first one.

So I see two solutions (cc @willdurand):

  • Reverse the order of the two bins in composer.json, which should fix the problem but is kinda hackish
  • Rewrite propel-gen to be a php script instead, and let composer create a propel-gen.bat proxy for you for windows users.
Owner

willdurand commented Feb 11, 2013

Thanks Jordi for your comment.

@staabm would you mind rewrite the propel-gen?

Member

staabm commented Feb 13, 2013

I would suggest to reverse the order of the binaries inside the composer.json in propel1 (as Pointed out by Seldaek) and do the rewrite for propel2...

But I am also willing to do the rewrite for propel1 in case you want me to do it.

Owner

marcj commented Feb 25, 2013

We should make a decision here. @staabm, if you're still willing to provide a PR then we should kinda fix that.

Member

staabm commented Feb 25, 2013

thats the way I would solve it:

I would suggest to reverse the order of the binaries inside the composer.json in propel1 (as Pointed out by Seldaek) and do the rewrite for propel2...

@marcj @jaugustin any opinions?

Owner

marcj commented Feb 25, 2013

Yes, I think that's probably the best solution, because

  • it's a fast solution, KISS
  • we don't need to override the whole bin/ scripts
  • I don't see its a hack, because actually composer shouldn't overwrite own repo files and we prevent that with this way

@ghost ghost assigned staabm Feb 25, 2013

Member

staabm commented Feb 25, 2013

Ok, will try to get a PR prepared for that.

Member

staabm commented Mar 3, 2013

@Seldaek I added the change you proposed.. could you tell me a way, how I could test this without merging it ..? don't know how to reference the propel version of this PR in a test-project to start testing...

Member

staabm commented Mar 3, 2013

@Seldaek Thank you.

tried using

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/staabm/propel"
        }
    ],
    "require": {
        "propel/propel1": "dev-issue588"
    }
}

but got

> composer update
Loading composer repositories with package information

  [RuntimeException]
  Failed to clone https://github.com/staabm/propel, could not read packages f
  rom it

  fatal: https://github.com/staabm/propel/info/refs?service=git-upload-pack n
  ot found: did you run git update-server-info on the server?

update [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-custom-instal
lers] [--no-scripts] [--no-progress] [-v|--verbose] [-o|--optimize-autoloader] [
packages1] ... [packagesN]
Contributor

Seldaek commented Mar 3, 2013

Seems like github is case sensitive, using
https://github.com/staabm/Propel (capital P) worked for me.

Member

staabm commented Mar 10, 2013

@Seldaek it works with an uppercased 'P' thanks for that.

The actual fix cd82fa3 you suggested didn't work for me. Although I re-ordered the propel-gen.bat composer generates a vendor/bin/propel-gen.bat which is not based on the one provided by propel.

Contributor

Seldaek commented Mar 11, 2013

OK I fixed this in composer/composer@0d06eb1 but it still breaks when executing the file anyway because it assumes phing.bat is in the PATH, which it isn't.

Member

staabm commented Mar 11, 2013

Thank you, will have another look at it this evening.

Member

staabm commented Mar 11, 2013

tried composer with your latest patch. The phing command is found but I runs in some errors because of path issues.

phing and the build.xml was found but not the required build.properties.

will investigate further..

Member

staabm commented Apr 15, 2013

no easy way to resolve this, without rewriting the bat from scratch.. closing

@staabm staabm closed this Apr 15, 2013

@xrstf xrstf added a commit to webvariants/datawrapper that referenced this pull request Jun 12, 2014

@xrstf xrstf rebuilt Propel files and added helper for Windows systems
Using Propel 1 with Composer is a nightmare on Windows:
propelorm/Propel#588

The small batch file works around Propel by simply directly
invoking Phing.
15ef3f5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment