Child shifter runs should only use the arguments provided #120

andrewnicols · 2014-04-02T05:23:53Z

When spawning a child Shifter run, we should not pass the child all of the
default arguments as these will override any specified in the build.json.

andrewnicols · 2014-04-02T05:24:45Z

We noticed this when doing a walk on a module which specifies lint: false in it's build.json.

andrewnicols · 2014-04-02T05:43:56Z

Looks like the travis build is broken downloading dependencies. Builds and tests fine locally on 0.10 for me though (and on 0.8 at travis-ci.org)

caridy · 2014-04-02T22:06:33Z

The issue with the build in nodejs 0.8 is due to this bug in travis: travis-ci/travis-ci#2076

andrewnicols · 2014-04-03T00:36:30Z

Ah - thanks. That explains it. I think I've seen that issue elsewhere too.

andrewnicols · 2014-04-09T15:47:12Z

Any thoughts on this issue @caridy?

caridy · 2014-04-09T17:02:21Z

lib/args.js

+        key;
+
+    // Get any supplied arguments if not specified.
+    if (args === undefined) {


testing for falsy value should be just fine.

caridy · 2014-04-09T17:08:14Z

I wonder if we could avoid changing the regular arguments initialization (setDefaults), and focus on fixing the arguments used to do the walk call? I'm worry that the changes in setDefaults() will affect some of the customers.

caridy · 2014-04-09T17:08:45Z

I will look into it later this week.

When spawning a child Shifter run, we should not pass the child all of the default arguments as these will override any specified in the build.json.

andrewnicols · 2014-04-11T07:52:38Z

I've changed the test to check for a falsy value, and reverted the setDefaults bit.

The reason I opted originally to modify setDefaults was to ensure that all of the basic options are at least defined reliably and consistently defined, whichever way you call Shifter. I was in two minds as to whether to include that change or not and I've now removed it.

andrewnicols · 2014-04-11T07:55:07Z

In writing this, I also discovered that it's possible to pass arguments to shifter which are not listed in the list of known arguments, and for shifter to use those somewhere. I do wonder whether we should address that too in another issue.

caridy · 2014-04-11T17:01:55Z

@andrewnicols this looks cleaner. thanks for making the changes, I will do a final pass on monday.

as for the not listed arguments, I'm reluctant to add more restrictions, or to make this a whitelisting process, I think it should continue to be a simple default set of attributes and basic validation.

…ols/shifter

This reverts commit 930273f, reversing changes made to 4199a12.

caridy · 2014-04-15T01:23:38Z

@andrewnicols I have to revert this one too. Shifter thru yogi reports issue with the arguments. The repro case is very easy:

npm link on yogi master
npm link on shifter feature branch
npm link shifter on yogi master
npm link yogi on yui
npm link shifter on yui
yogi build

andrewnicols · 2014-04-15T01:24:48Z

Darn - I even tried this with Yogi, but I'd forgotten to link shifter on my yogi branch. Will take a peek.

Thanks!

andrewnicols · 2014-04-15T01:37:16Z

Hmm @caridy, can I ask what you're seeing? It's working perfectly for me with your replication instructions:

2011 nicols@loganberry:~/git/yui> cd yogi/
2012 yogi:master> npm link
npm WARN package.json [email protected] 'repositories' (plural) Not supported. Please pick one as the 'repository' field
npm WARN package.json [email protected] No repository field.
npm WARN package.json [email protected] No repository field.
/usr/local/bin/yogi -> /usr/local/lib/node_modules/yogi/bin/yogi.js
/usr/local/lib/node_modules/yogi -> /Users/nicols/git/yui/yogi
2013 yogi:master> cd ../shifter/
2014 shifter:walk_options> npm link
npm WARN package.json [email protected] 'repositories' (plural) Not supported. Please pick one as the 'repository' field
npm WARN package.json [email protected] No repository field.
npm WARN package.json [email protected] No repository field.
npm WARN package.json [email protected] No repository field.
/usr/local/bin/shifter -> /usr/local/lib/node_modules/shifter/bin/shifter
/usr/local/lib/node_modules/shifter -> /Users/nicols/git/yui/shifter
2015 shifter:walk_options> cd ../yogi/
2016 yogi:master> npm link shifter
unbuild [email protected]
/Users/nicols/git/yui/yogi/node_modules/shifter -> /usr/local/lib/node_modules/shifter -> /Users/nicols/git/yui/shifter
2017 yogi:master> cd ../yui3
n2018 yui3:dev-3.x> npm link yogi
unbuild [email protected]
npm WARN prefer global [email protected] should be installed with -g
/Users/nicols/git/yui/yui3/node_modules/yogi -> /usr/local/lib/node_modules/yogi -> /Users/nicols/git/yui/yogi
2019 yui3:dev-3.x> npm link shifter
unbuild [email protected]
npm WARN prefer global [email protected] should be installed with -g
/Users/nicols/git/yui/yui3/node_modules/shifter -> /usr/local/lib/node_modules/shifter -> /Users/nicols/git/yui/shifter
2020 yui3:dev-3.x> yogi build
yogi [info]  using [email protected] on [email protected]
yogi [info]  running with shifter
shifter [info] racing to find the closest .shifter.json file
shifter [info] revving up
shifter [info] racing the directories
shifter [info] using --no-lint
shifter [info] found 102 modules to race, let's do this
shifter [warn] this will be quiet, only status will be emitted for speed. failed builds will print after
......................................................................................................
shifter [info] done racing, the gears are toast
shifter [info] finished in 1 minute, 26 seconds, pretty fast huh?
yogi [info]  build complete

caridy · 2014-04-15T17:05:56Z

Ok, can you issue a new PR so I can properly test it? maybe I messed up with the merges.

caridy · 2014-04-16T23:46:45Z

@andrewnicols it might be PR #115, this is what I'm getting with the current master branch:

cleanglob-lm:yui3 caridy$ yogi build
yogi [info]  using [email protected] on [email protected]
yogi [info]  running with shifter
shifter [info] racing to find the closest .shifter.json file
shifter [info] revving up
shifter [info] racing the directories
shifter [info] using --no-lint
shifter [info] found 102 modules to race, let's do this
shifter [warn] this will be quiet, only status will be emitted for speed. failed builds will print after
....shifter [err] [ [ , , , [ , , , , [Object], [Object] ] ] ]
!..........shifter [err] [ , , [ , , , [ , , , , [Object], [Object] ] ] ]
!...............shifter [err] [ , , , , [ , , , [ , , , , [Object] ] ] ]
!...........shifter [err] [ [ , , , [ , , , , [Object] ] ] ]
!..........................................................
shifter [info] done racing, the gears are toast
shifter [info] finished in 1 minute, 18 seconds, pretty fast huh?
shifter [warn] the following builds exited with a 1
    widget-stack
    widget
    scrollview
    overlay
shifter [err] Walk failed, 4 builds exited with a 1
yogi [info]  build complete
yogi [ERROR] yogi detected an error, exiting code 1

ideally, you, @ianstigator and @unkillbob can help me out here because I honestly don't have too much time these days. Master is currently stable, tests are passing, but when trying with the steps I described above, I see that weird error. If you can issue a PR with the fix, I will be happy to merge it and release the new version of shifter. otherwise it might take me a while to revisit this.

andrewnicols · 2014-04-17T02:08:59Z

Okay, I'm seeing these issues coming from the buildCallback argument introduced in #115.
Running on any of the above affected modules (widget-stack, widget, scrollview, overlay) without walk also displays it.
That's what's also printing out

shifter [err] [ [ , , , [ , , , , [Object], [Object] ] ] ]

@ianstigator @unkillbob

andrewnicols · 2014-04-17T02:31:20Z

I've just pulled a clean copy of v0.4.6 and applied #118 and #120 - yogi builds are fine.
It's definitely the inclusion of the callbacks that break things in #115.

The output (shifter [err] [ [ , , , [ , , , , [Object], [Object] ] ] ]) is being printed by logAndExit.

The problems seem to start in https://github.com/orionhealth/shifter/commit/6f2e135e5cbab1be55695bedee807da094785b59 but it could be that it's highlighting a pre-existing issue.

Indeed, I think it may be. I've printed out the two objects in the err (array), along with a stacktrace:

Error
    at logAndExit (/Users/nicols/git/yui/shifter/lib/index.js:51:21)
    at /Users/nicols/git/yui/shifter/lib/index.js:179:33
    at end (/Users/nicols/git/yui/shifter/lib/builder.js:89:17)
    at /Users/nicols/git/yui/shifter/lib/builder.js:186:20
    at Object.Stack.test (/Users/nicols/git/yui/shifter/lib/stack.js:31:27)
    at /Users/nicols/git/yui/shifter/lib/stack.js:25:18
    at end (/Users/nicols/git/yui/shifter/lib/module.js:822:13)
    at /Users/nicols/git/yui/shifter/lib/module.js:749:13
    at Object.Stack.test (/Users/nicols/git/yui/shifter/lib/stack.js:31:27)
    at /Users/nicols/git/yui/shifter/lib/stack.js:25:18
[ ,
  ,
  ,
  [ ,
    ,
    ,
    ,
    { task: 'check',
      error: 'writing zero length file from /Users/nicols/git/yui/yui3/src/widget-stack/assets/skins/night/widget-stack-skin.css' },
    { task: 'check',
      error: 'writing zero length file from /Users/nicols/git/yui/yui3/src/widget-stack/assets/skins/sam/widget-stack-skin.css' } ] ]

Both of the input files there are empty. As a result, the task checker reports that the output files are empty too and bails. Fair enough... Commenting out the zero length test stops the yogi build from failing.

So I'm seeing two issues we need to resolve here:

lib/tasks/check.js needs to either be aware of zero-length input files or not be called when nothing was done; and
log.err needs to be able to print useful information rather than objects in an array from lib/tasks/check.js

andrewnicols · 2014-04-17T02:52:15Z

@caridy, you mentioned that you needed me to supply a new merge branch as you'd reverted my branch, but I'm not seeing that in the history.
My changes are still present in https://github.com/yui/shifter/blob/master/lib/args.js#L70 and https://github.com/yui/shifter/blob/master/lib/index.js#L217

There is a revert commit 92dd95a, and the comment suggests that you're reverting #120 and #118, but from what I can see, that's #115 and #118.

caridy · 2014-04-17T13:52:33Z

@andrewnicols I only reverted PR #118 based on your comments and your previous investigation. Your branch and this branch are in as in master today.

caridy · 2014-06-11T23:06:07Z

[email protected]

caridy reviewed Apr 9, 2014
View reviewed changes

Child shifter runs should only use the arguments provided

fd08a3e

When spawning a child Shifter run, we should not pass the child all of the default arguments as these will override any specified in the build.json.

caridy merged commit fd08a3e into yui:master Apr 15, 2014

caridy added a commit that referenced this pull request Apr 15, 2014

PR #120 - Merge branch 'walk_options' of https://github.com/andrewnic…

1227ffe

…ols/shifter

caridy added a commit that referenced this pull request Apr 15, 2014

Revert PR #120 and PR #118 due to issues when using shifter thru yogi

92dd95a

This reverts commit 930273f, reversing changes made to 4199a12.

caridy mentioned this pull request Apr 15, 2014

Callback on error instead of process.exit when Shifter is called programmatically #115

Merged

2 tasks

This was referenced Apr 17, 2014

Check the size of source files in the output file size check #121

Closed

Build fail issues #122

Open

caridy added a commit that referenced this pull request Jun 11, 2014

bumping version 0.5.0 for PR #131, #128, #115 and #120

21916ef

Child shifter runs should only use the arguments provided #120

Child shifter runs should only use the arguments provided #120

Uh oh!

Conversation

andrewnicols commented Apr 2, 2014

Uh oh!

andrewnicols commented Apr 2, 2014

Uh oh!

andrewnicols commented Apr 2, 2014

Uh oh!

caridy commented Apr 2, 2014

Uh oh!

andrewnicols commented Apr 3, 2014

Uh oh!

andrewnicols commented Apr 9, 2014

Uh oh!

caridy Apr 9, 2014

Choose a reason for hiding this comment

Uh oh!

caridy commented Apr 9, 2014

Uh oh!

caridy commented Apr 9, 2014

Uh oh!

andrewnicols commented Apr 11, 2014

Uh oh!

andrewnicols commented Apr 11, 2014

Uh oh!

caridy commented Apr 11, 2014

Uh oh!

caridy commented Apr 15, 2014

Uh oh!

andrewnicols commented Apr 15, 2014

Uh oh!

andrewnicols commented Apr 15, 2014

Uh oh!

caridy commented Apr 15, 2014

Uh oh!

caridy commented Apr 16, 2014

Uh oh!

andrewnicols commented Apr 17, 2014

Uh oh!

andrewnicols commented Apr 17, 2014

Uh oh!

andrewnicols commented Apr 17, 2014

Uh oh!

caridy commented Apr 17, 2014

Uh oh!

caridy commented Jun 11, 2014

Uh oh!

Uh oh!