Skip to content

Commit

Permalink
prevent installing grunt-appdmg on linux/win
Browse files Browse the repository at this point in the history 
created issue here:
rakuten-frontend/grunt-appdmg#2
  • Loading branch information
@shaunlebron
shaunlebron committed Mar 21, 2015
1 parent 05f864d commit 646fade
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 1 deletion.
3 changes: 3 additions & 0 deletions scripts/setup.bat
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
pushd "%~dp0\.."

echo. & echo Installing node dependencies...
rem "grunt-appdmg" is a mac-only dependency that will fail to build on windows.
rem So we are including it as an optionalDependency in package.json
rem and preventing its installation with npm's --no-optional flag.
call npm install

echo. & echo Installing grunt...
Expand Down
10 changes: 9 additions & 1 deletion scripts/setup.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,15 @@ set -e
cd "`dirname $0`/.."

echo; echo "Installing node dependencies..."
npm install

if [[ "$OSTYPE" == "darwin"* ]]; then
npm install
else
# "grunt-appdmg" is a mac-only dependency that will fail to build on linux.
# So we are including it as an optionalDependency in package.json
# and preventing its installation with npm's --no-optional flag.
npm install --no-optional
fi

echo; echo "Installing grunt..."
sudo npm install -g grunt-cli
Expand Down

7 comments on commit 646fade

@ducky427
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should installNodeDepsToRelease function in Gruntfile.js also be fixed to address this?

@shaunlebron
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ducky427 thanks, I just ran a test and I don't think we need to fix it. grunt release on my mac didn't create appdmg in the release folder's node_modules, so npm install --production must be ignoring optional dependencies. (cc: @oakmac)

@ducky427
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @shaunlebron.

I checked on a linux box and npm install --production does indeed try to install grunt-appdmg. Looking at the documentation for npm install, it looks like --production, doesn't install devDependencies. So by implication, it does install optionalDependencies.

My solution is in installNodeDepsToRelease have the following code

  if (os === "mac") {
    exec('npm install --production'); 
  } else {
    exec('npm install --no-optional --production');
  }

@shaunlebron
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ducky427 Hmm, thanks for verifying that. I think we can actually just do --no-optional regardless of the OS, since we're not trying to install appdmg in the release directory for any OS.

Still, the disconnect between npm install --production not installing appdmg on mac and yet still failing on linux is strange to me.

@shaunlebron
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ducky427 commited: 1819f49

@shaunlebron
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ducky427 thanks again, next time I think this'll be better done with a PR

@ducky427
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @shaunlebron. Agreed this is better done as a PR!

👍

Please sign in to comment.