Skip to content
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

Cordova iOS 5.0.0 can not build without node_modules present because of missing shelljs for build phase script #540

Closed
3 tasks done
janpio opened this issue Feb 19, 2019 · 7 comments
Assignees
Milestone

Comments

@janpio
Copy link
Member

janpio commented Feb 19, 2019

Bug Report

Problem

What is expected to happen?

The Cordova iOS project used to be able to build outside of a Cordova project. You could just copy platforms/ios and then use Xcode or xcodebuild to build your app.

What does actually happen?

You can not do that any more as the shelljs dependency is used by one of the build step scripts and is not present without the parent node_modules.

Information

As soon as node_modules from the main project folder is missing (because it was either not created after a checkout from git, or because you copied over the iOS project from platforms/ios to somewhere else, you get an error like this when trying to build with xcodebuild or Xcode:

MacBook-Pro:ios sujan$ xcodebuild -workspace HelloCordova.xcworkspace -scheme HelloCordova -configuration Debug -sdk iphonesimulator -destination platform="iOS Simulator,name=iPhone X" build CONFIGURATION_BUILD_DIR=/Users/sujan/Projects/throwaway/cordovaIos5Test2/platforms/ios/build/emulator SHARED_PRECOMPS_DIR=/Users/sujan/Projects/throwaway/cordovaIos5Test2/platforms/ios/build/sharedpch
Build settings from command line:
    CONFIGURATION_BUILD_DIR = /Users/sujan/Projects/throwaway/cordovaIos5Test2/platforms/ios/build/emulator
    SDKROOT = iphonesimulator12.1
    SHARED_PRECOMPS_DIR = /Users/sujan/Projects/throwaway/cordovaIos5Test2/platforms/ios/build/sharedpch

note: Using new build system
note: Planning build
note: Constructing build description

[...]

PhaseScriptExecution Copy\ www\ directory /Users/sujan/Library/Developer/Xcode/DerivedData/HelloCordova-gnnlxcyurmjjpncmcbzgevousltv/Build/Intermediates.noindex/HelloCordova.build/Debug-iphonesimulator/HelloCordova.build/Script-304B58A110DAC018002A0835.sh (in target: HelloCordova)
    cd /Users/sujan/Projects/throwaway/cordovaIos5Test2/platforms/ios
    /bin/sh -c /Users/sujan/Library/Developer/Xcode/DerivedData/HelloCordova-gnnlxcyurmjjpncmcbzgevousltv/Build/Intermediates.noindex/HelloCordova.build/Debug-iphonesimulator/HelloCordova.build/Script-304B58A110DAC018002A0835.sh
module.js:338
    throw err;
    ^

Error: Cannot find module 'shelljs'
    at Function.Module._resolveFilename (module.js:336:15)
    at Function.Module._load (module.js:286:25)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/sujan/Projects/throwaway/cordovaIos5Test2/platforms/ios/cordova/lib/copy-www-build-step.js:34:13)
    at Module._compile (module.js:434:26)
    at Object.Module._extensions..js (module.js:452:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:475:10)
Command PhaseScriptExecution failed with a nonzero exit code

** BUILD FAILED **


The following build commands failed:
	PhaseScriptExecution Copy\ www\ directory /Users/sujan/Library/Developer/Xcode/DerivedData/HelloCordova-gnnlxcyurmjjpncmcbzgevousltv/Build/Intermediates.noindex/HelloCordova.build/Debug-iphonesimulator/HelloCordova.build/Script-304B58A110DAC018002A0835.sh
(1 failure)
MacBook-Pro:ios sujan$ 

(The error in Xcode directly is identical, as it just uses xcodebuild under the hood.)

Known workaround

You can work around this problem by running npm install shelljs in your iOS project. This currently seems to be the only dependency required during build.

How could this be solved?

Dependencies that are necessary for the scripts included in the project itself could/should be included in the actual project. (Alternatively there could be a package.json that is not installed by default, so it becomes at least possible to discover this when looking around)

Command or Code

cordova create testProject
cd testProject
cordova platform add cordova-ios@5.0.0
cordova build ios
# now copy the `xcodebuild` command (and add `"` around the `-destination platform=` parameter as this is missing from the command output)
cd platforms/ios
xcodebuild ... # command from before => works
rm ../../node_modules
xcodebuild ... # command from before => fails with error shown above

Environment, Platform, Device

macOS 10.14.2 (Mojave)

Version information

Cordova CLI 8.1.1, Cordova iOS 5.0.0, Xcode 10.1 (10B61)

Checklist

  • I searched for existing GitHub issues
  • I updated all Cordova tooling to most recent version
  • I included all the necessary information above
@janpio
Copy link
Member Author

janpio commented Feb 19, 2019

Some context on copy-www-build-step.js:
This is a script that is used for the "Copy www directory" build phase of the Xcode project. You can find the command behind its usage by opening the Xcode project, select the target, then "Build phases" tab, then "Copy www directory" phase. Here you have a command that is executed via /bin/sh.

Took me a bit to understand that construct.

@janpio
Copy link
Member Author

janpio commented Feb 20, 2019

Ok, talked to @erisu a bit about this:

There are multiple options to fix this:

  1. Do what I said above and bring the dependencies back. Bad as you don't want to have bundled dependencies and all the problems that come with it.
  2. We could change back the script from node to being a shell script, which it seems it was a few years ago (see https://github.com/apache/cordova-ios/blob/3.8.x/bin/templates/scripts/cordova/lib/copy-www-build-step.sh). Probably not the best idea, as there sure was a reason why we change it back then.
  3. We can change the script from shelljs to something that comes with node, e.g. child_process and similar. As this script only runs on macOS, no need for cross platform support etc.

So option 3 it is I guess.

@dpogue
Copy link
Member

dpogue commented Feb 25, 2019

I think my preference is for option 2. The script isn't complicated, and consists mostly of shelling out to run various commands, so there's no compelling reason in my mind for it to not just be a bash script.
There's also a lot of effort that goes into making sure it runs with the right node executable, and in theory a bash script would avoid all of that hassle.

@janpio
Copy link
Member Author

janpio commented Feb 25, 2019

Good point. I wonder why this was changed in the first place... Will have to look at the blame and file history a bit.

@janpio
Copy link
Member Author

janpio commented Mar 1, 2019

The file is here: https://github.com/apache/cordova-ios/blob/master/bin/templates/scripts/cordova/lib/copy-www-build-step.js

History is here: https://github.com/apache/cordova-ios/blame/master/bin/templates/scripts/cordova/lib/copy-www-build-step.js

Changes to the .js file are pretty limited and small:

#146 is the original PR that switched the script from bash to node, which resolved this issue: https://issues.apache.org/jira/browse/CB-8197

Currently platform tooling for ios is based on bash scripts and to improve maintainability of such scripts it would be useful to port them to NodeJS.

This is the last version of the shellscript before it was removed via that PR: https://github.com/apache/cordova-ios/blob/f8a5710cbafebfedc000335152777bc307e8b7ac/bin/templates/scripts/cordova/lib/copy-www-build-step.sh

The PR didn't have the "lot of effort that goes into making sure it runs with the right node executable" yet, so this was added in a later PR.

@janpio
Copy link
Member Author

janpio commented Mar 1, 2019

Yep, this is the PR that then complicated the node call: 57d3dcd JIRA issue and discussion is here: https://issues.apache.org/jira/browse/CB-9273

@janpio
Copy link
Member Author

janpio commented Mar 1, 2019

And that is where my sleuthing ends, I am not qualified to decide if we should adapt the old .sh and just add the changes done to the .js or write a new shell script. In generall I have no idea how shell scripts work, so I am pretty lost. Can you take a shot at it please @dpogue?

@dpogue dpogue added this to the 5.0.1 milestone Mar 15, 2019
@dpogue dpogue self-assigned this Mar 25, 2019
dpogue added a commit to dpogue/cordova-ios that referenced this issue Apr 16, 2019
There didn't seem to be a compelling reason to keep this as JavaScript
since it was primarily just shelling out to run various commands. By
keeping it in bash, we can significantly simplify the invocation in the
xcodeproj file and we can run it without a dependency on shelljs.

I also moved it into a Scripts folder inside the project so that our
xcodeproj file isn't reliant on the cordova/lib folder existing, and
this particular script isn't actually a "lib" in the standard sense.
dpogue added a commit that referenced this issue Apr 17, 2019
There didn't seem to be a compelling reason to keep this as JavaScript
since it was primarily just shelling out to run various commands. By
keeping it in bash, we can significantly simplify the invocation in the
xcodeproj file and we can run it without a dependency on shelljs.

I also moved it into a Scripts folder inside the project so that our
xcodeproj file isn't reliant on the cordova/lib folder existing, and
this particular script isn't actually a "lib" in the standard sense.
@dpogue dpogue closed this as completed Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants