So I suppose we should
1. remove .bat extension from other script locations to be consistent, for
example
'cordova-wp8':
{ 'platform':'wp8', 'scriptSrc':
path.join(project_dir,'cordova','version.bat') },
2. Remove check for the file location on Windows before executing it due to
additional different file extensions (we don't know exact target name, but
.exec or .spawn cmd /c will be able to run it)
if(fs.existsSync(engine.scriptSrc)){
fs.chmodSync(engine.scriptSrc, '755');
var d = Q.defer();
child_process.exec(engine.scriptSrc, function(error, stdout,
stderr) {
3. If we are switching to spawn we may want to wrap spawn functionality to
special module which will hide all platform specific checks/magic
+ 1 for cmd /c due to same reason
Are you ok if I create ticket for this specific bug (plugman + wp78) and fix it
as per steps 1 and 2 above?
Thx!
Sergey
-----Original Message-----
From: Carlos Santana [mailto:csantan...@gmail.com]
Sent: Wednesday, October 23, 2013 10:23 PM
To: dev@cordova.apache.org
Subject: Re: Plugman engine check and WP7/8
Actually just try it out and see that using spawn with "cmd"
["/c","cordova/build",...
is better option than adding the ".bat" then it covers "build.exe",
"build.bat", and "build.cmd" on windows
if someone thinks this is bad route, please shime in this jira issue [1]
[1]: https://issues.apache.org/jira/browse/CB-5187
--Carlos
On Wed, Oct 23, 2013 at 1:23 PM, Carlos Santana <csantan...@gmail.com>wrote:
> Thanks Bryan for pointing out;
> I created a jira issue to address the "compile, emualate, run" using
> spawn and not addressing windows ".bat"
>
> https://issues.apache.org/jira/browse/CB-5187
>
> I'm guessing the fix is to detect that node environment is Windows and
> append ".bat" to cmd for child process spawn We can't use exec, that
> was the original problem why we moved to spawn per Braden suggestion
> about stdio
>
> Not sure what to say about the version topic being discuss, maybe
> someone with knowledge about the problem should open a jira issue
>
> I'm out on a 2 day conference, will try to land something today unless
> wants to take over and help.
>
>
> --Carlos
>
>
>
> On Wed, Oct 23, 2013 at 11:06 AM, Bryan Higgins <br...@bryanhiggins.net>wrote:
>
>> I know Carlos just changed several CLI commands to use spawn. There
>> doesn't seem to be any specific handling for Windows.
>>
>>
>> https://git-wip-us.apache.org/repos/asf?p=cordova-cli.git;a=commit;h=
>> 01c7ecec7ccf4a3c1423ddf3844e125d24965025
>>
>>
>> On Wed, Oct 23, 2013 at 10:44 AM, Braden Shepherdson
>> <bra...@chromium.org
>> >wrote:
>>
>> > I thought we were shelling out that command, not trying to load the
>> file.
>> > Then Windows would locate the version.bat file and run it, while
>> > Unixy platforms would see the version file with +x, and run it.
>> >
>> > Are we no longer going through the shell? (This should be using
>> > child_process.exec, not .spawn, for example.)
>> >
>> > Braden
>> >
>> >
>> > On Wed, Oct 23, 2013 at 10:13 AM, Bryan Higgins <
>> bhigg...@blackberry.com
>> > >wrote:
>> >
>> > > This issue has to do with the host system rather than platform.
>> Android
>> > and
>> > > BB10 both have version.bat files.
>> > >
>> > >
>> > > On Wed, Oct 23, 2013 at 9:56 AM, Sergey Grebnov (Akvelon) <
>> > > v-seg...@microsoft.com> wrote:
>> > >
>> > > > Hi,
>> > > >
>> > > > #1 The problem
>> > > > Right now the simplest (and also the most correct IMO) way to
>> specify
>> > > > plugin restrictions to specific cordova version is the following:
>> > > >
>> > > > plugin.xml:
>> > > >
>> > > > <engines>
>> > > > <engine name="cordova" version=">=2.7.0" /> </engines>
>> > > >
>> > > > But in this case as per plugman engines definition
>> > > > (plugman/src/util/default-engines.js) plugman will always try
>> > > > to
>> find
>> > > > version verification script in some predefined location, not
>> > > > taking
>> > into
>> > > > account currently running platform, and will fail on WP since
>> correct
>> > > > script name is version.bat, not just version.
>> > > >
>> > > > module.exports = function(project_dir){
>> > > > return {
>> > > > 'cordova':
>> > > > { 'platform':'*', 'scriptSrc':
>> > > > path.join(project_dir,'cordova','version') }, <- works in
>> > > > general,
>> but
>> > > NOT
>> > > > for WP7/8
>> > > > ...
>> > > > 'cordova-wp8':
>> > > > { 'platform':'wp8', 'scriptSrc':
>> > > > path.join(project_dir,'cordova','version.bat') }, <- correct
>> location,
>> > > not
>> > > > used in case of example above
>> > > >
>> > > >
>> > > > This means that right now there is no way to specify platform
>> dependent
>> > > > location of version verification script for 'cordova' engine
>> > > > check
>> > which
>> > > > is going to be the most popular.
>> > > >
>> > > > #2 Proposed solution
>> > > >
>> > > > Taking into account we have platform context when we are
>> > > > looking for
>> > the
>> > > > appropriate engine
>> > > > plugman/src/install.js
>> > > > function getEngines(pluginElement, platform,
>> > > > project_dir, plugin_dir){
>> > > >
>> > > > I propose to think about 'cordova' engine settings (in
>> > > default-engines.js)
>> > > > as a fallback in case we don't have any platform specific
>> > > > engine for
>> > some
>> > > > platform. So in case of engine.attrib["name"] == 'cordova' we
>> should
>> > > check
>> > > > if there is engine with ['cordova-' + platform] name first and
>> > > > if it
>> > does
>> > > > not exist use 'cordova' engine settings only. For example we
>> already
>> > do
>> > > > this by the following line, but we need both engines (cordova
>> > > > and
>> > > > cordova-wp8) specified in plugin.xml file. Thoughts?
>> > > >
>> > > > // make sure we check for platform req's and not just cordova reqs
>> > > > if(cordovaEngineIndex && cordovaPlatformEngineIndex)
>> > > > uncheckedEngines.pop(cordovaEngineIndex);
>> > > >
>> > > > PS. Another minor potential issue seems to be in check above;
>> > > > cordovaEngineIndex && cordovaPlatformEngineIndex will return
>> > > > false
>> if
>> > one
>> > > > of indexes is zero (zero is valid/correct index in this context
>> > > > so
>> it
>> > > must
>> > > > be true).
>> > > >
>> > > > Thx!
>> > > > Sergey
>> > > >
>> > >
>> >
>>
>
>
>
> --
> Carlos Santana
> <csantan...@gmail.com>
>
--
Carlos Santana
<csantan...@gmail.com>
