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.

Sign up for GitHub

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

[ZEPPELIN-1928]: Get a rid of preinstalled external dependencies (nodejs and npm) for maven build #2002

Closed
wants to merge 10 commits into from
Closed

[ZEPPELIN-1928]: Get a rid of preinstalled external dependencies (nodejs and npm) for maven build #2002

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d09f9b3
[ZEPPELIN-1928] Get a rid of preinstalled external dependencies (node…
DmytroShkvyra Feb 9, 2017
337bc89
[ZEPPELIN-1928] add rat exclusion
DmytroShkvyra Feb 9, 2017
eec761f
[ZEPPELIN-1928] add execution permition for add_dependencies.sh
DmytroShkvyra Feb 9, 2017
e730dff
[ZEPPELIN-1928] optomization
DmytroShkvyra Feb 13, 2017
7a160de
[ZEPPELIN-1928] update dock
DmytroShkvyra Feb 13, 2017
07cef04
[ZEPPELIN-1928] fix rat error and phases of nodejs (yarn and bower) i…
DmytroShkvyra Feb 14, 2017
a4210b3
[ZEPPELIN-1928] change profile activation
DmytroShkvyra Feb 14, 2017
49506a8
[ZEPPELIN-1928] remove external dependency installation
DmytroShkvyra Feb 15, 2017
aceb92d
[ZEPPELIN-1928] remove rat exclusion
DmytroShkvyra Feb 15, 2017
2ed710f
[ZEPPELIN-1928] fix formatting
DmytroShkvyra Feb 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions docs/install/build.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,6 @@ If you don't have requirements prepared, install it.
sudo apt-get update
sudo apt-get install git
sudo apt-get install openjdk-7-jdk
sudo apt-get install npm
sudo apt-get install libfontconfig
```


Expand Down
14 changes: 11 additions & 3 deletions zeppelin-web/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,27 @@ It is recommended to install node 6.0.0+ since Zeppelin uses 6.9.1+ (see [creati

All build commands are described in [package.json](./package.json)

```sh
# install required depepdencies and bower packages (only once)
$ npm install -g yarn
$ yarn install
```
$ npm install -g yarn bower npm-run-all rimraf grunt webpack karma karma-phantomjs-launcher
Copy link
Member

Choose a reason for hiding this comment

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

I'm don't think 'rimraf grunt webpack karma karma-phantomjs-launcher' are really a required package installed globally for development. Shell we remove them?

Also shell we not use 'npm-run-all' package?
After this PR, every existing developer will face error, such as

$ npm-run-all --parallel dev:server dev:watch 
sh: npm-run-all: command not found
error Command failed with exit code 127.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

and all developer need to read README.md again and install necessary package. which is big impact compare to benefit that we can get from npm-run-all here.

What do you think?

Copy link
Member

@1ambda 1ambda Feb 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@DmytroShkvyra We don't need to install these packages as global. All packages installed locally (node_modules) can be used in scripts tag in package.json as command like grunt, webpack, karma... except yarn which is executed by users directly in terminal.

Copy link
Member

@1ambda 1ambda Feb 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@DmytroShkvyra

Here is doc for it - https://docs.npmjs.com/misc/scripts#path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for doc reference about call script with relative path. In my opinion this is will be a problem because path delimeters are different for Linux and Windows and we should not make zeppelin is platform dependent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Leemoonsoo

I'm don't think 'rimraf grunt webpack karma karma-phantomjs-launcher' are really a required >package installed globally for development. Shell we remove them?
Also shell we not use 'npm-run-all' package?
Answer - I don't know because this not my code and as far as I understand this code was developed for local development by UI developers whom should care about they local env.
I just tested process which was described in /zeppelin/zeppelin-web/README.md and found out that even after

$ mvn package

I was had to install rimraf grunt webpack karma karma-phantomjs-launcher globally for successful build

yarn build

So I just changed /zeppelin/zeppelin-web/README.md to real state of things.
If you disagree with this i can revert README.md because local UI developmen out of scope ZEPPELIN-1928

Copy link
Member

Choose a reason for hiding this comment

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

@DmytroShkvyra I see. Which environment did you use when it requires install 'rimraf grunt webpack karma karma-phantomjs-launcher' globally?

Because on linux, osx, mvn package success without installing them globally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Leemoonsoo Please try build project at clear box (without reinstalled nodejs and npm), and you got a lot of failures at zeppelin-web. And try my patch...

```

```
$ yarn install --no-lockfile --no-bin-links
```
# build zeppelin-web for production
```
$ yarn run build
```

# run frontend application only in dev mode (localhost:9000)
# you need to run zeppelin backend instance also
```
$ yarn run dev
```

# execute tests
```
$ yarn run test
```

Expand Down
2 changes: 2 additions & 0 deletions zeppelin-web/add_dependencies.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
#!/bin/bash
( sudo apt-get install libfontconfig -y || sudo yum install fontconfig -y ) || exit 0;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's best idea install package on the system without asking user. Shell we remove this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest this is not the best solution. The best solution would be present all dependencies in phantomjs for all planforms. But this workaround just work in linux and provide automatical build with maven and not required from e.g. (backend developers and wondows users) to be familiar with hodejs npm yarn and other UI-specific things like hidden dependencies and so on.

Copy link
Member

@Leemoonsoo Leemoonsoo Feb 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Then how about print message "Please install xxxxx with apt-get install xxxxx command" when required libraries are not available on the system?

I still think installing package without asking user is not really a good idea. And we can't assume that user will build Zeppelin with 'sudo' enabled account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Leemoonsoo Mmm, the main goal of this PR is give possibility run

mvn clean package -Pbuild-distr

(see http://zeppelin.apache.org/docs/snapshot/install/build.html#package)
without failures. As you can see in my PR (screenshot section) zeppelin-web builds about 40 minutes. So if hidden dependency was not installed automatically build will be failed after about 50 minutes. And only after this i could show this message. I think this is vesting of time and money (if you use EC2). More over in this case user should waiting for this a front of monitor...
This is normal situation when linux package already installed and user try install it again - this will not changed system state.
BTW, zeppelin developed not only at Ubuntu nodes. What should we do if this is Redhat or Centos or OsX? Fortunately, Windows phantomjs package do not need these hidden dependencies, but Lunuxes need. I suspect that OsX also...

Copy link
Member

Choose a reason for hiding this comment

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

Then how about adjust build order? Move 'zeppelin-web' module to the front of build order, so it can fail early?

We never know user want to (or allowed to) install any package on the system or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Leemoonsoo I can adjust build order but this do not save enough time - its run about 40 minutes until fail. To be honest I don't understand why you so opposite to automatically install package which should be already installed for build? If you have newer version at your workstation package manager will not reinstall old package. This is a safe operation.

5 changes: 2 additions & 3 deletions zeppelin-web/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,15 @@
},
"scripts": {
"clean": "rimraf dist && rimraf .tmp",
"postinstall": "bower install --silent",
"prebuild": "npm-run-all clean",
"prebuild": "bower install --allow-root --silent && npm-run-all clean",
"build": "grunt pre-webpack-dist && webpack && grunt post-webpack-dist",
"predev": "grunt pre-webpack-dev",
"dev:server": "webpack-dev-server --hot",
"dev:helium": "HELIUM_BUNDLE_DEV=true webpack-dev-server --hot",
"dev:watch": "grunt watch-webpack-dev",
"dev": "npm-run-all --parallel dev:server dev:watch",
"visdev": "npm-run-all --parallel visdev:server dev:watch",
"pretest": "npm install karma-phantomjs-launcher",
"pretest": "npm install karma-phantomjs-launcher --no-lockfile --no-bin-links",
"test": "karma start test/karma.conf.js"
},
"dependencies": {
Expand Down
128 changes: 106 additions & 22 deletions zeppelin-web/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,37 @@
<!--plugin versions-->
<plugin.frontned.version>1.3</plugin.frontned.version>
</properties>

<profiles>
<profile>
<id>linux_profile</id>
<activation>
<os>
<family>linux</family>
</os>
</activation>
<build>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>exec-maven-plugin</artifactId>
<executions>
<execution>
<id>some-execution</id>
<phase>process-test-resources</phase>
<goals>
<goal>exec</goal>
</goals>
</execution>
</executions>
<configuration>
<executable>${basedir}/add_dependencies.sh</executable>
</configuration>
</plugin>
</plugins>
</build>
</profile>
</profiles>

<build>
<plugins>
Expand Down Expand Up @@ -98,6 +129,7 @@
<exclude>**/.npmignore</exclude>
<exclude>yarn.lock</exclude>
<exclude>*.md</exclude>
<exclude>*.sh</exclude>
</excludes>
</configuration>
</plugin>
Expand All @@ -120,34 +152,77 @@
</execution>

<execution>
<id>yarn install</id>
<goals>
<goal>yarn</goal>
</goals>
<configuration>
<arguments>install --no-lockfile</arguments>
</configuration>
<id>yarn add bower</id>
<goals>
<goal>yarn</goal>
</goals>
<configuration>
<!--ignore-scripts-->
<arguments>add bower --no-lockfile --no-bin-links</arguments>
Copy link
Member

Choose a reason for hiding this comment

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

we don't need to run

'yarn add bower'
'yarn add karma-phantomjs-launcher'

and we can just run command defined in 'script' field in 'package.json'.

like

'yarn install'
'yarn build'
'yarn test'

we don't want to define the same command in the pom.xml that 'package.json' already has in 'script' field, such as

'bower install'
'webpack build'
'grunt pre-webpack-dist'
'grunt post-webpack-dist'
'karma test'

I think all changes made in 'executions' section of 'frontend-maven-plugin' are unnecessary and should be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be you know better... My main goal is to make developers free from local env for start development and build. Also i wanted give easier ability newbies try zeppelin. I don't care about how to UI developers will run these scripts locally.
They can run this as they used to. I have just changed "postinstall" trigger in package.json because it interfered with "yarn install" from frontend-maven-plugin execution (yarn install). Do you want remove all changes in *.MD or remove all changes in pom and package.json?

Copy link
Member

Choose a reason for hiding this comment

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

My main goal is to make developers free from local env for start development and build.

I assume you meant "local env" -> "local env other than osx, linux". Because at least, osx, linux doesn't require any additional steps to build 'zeppelin-web' module except for just run 'mvn package'.

Like i mentioned above, run 'yarn add xx' changes 'package.json', which is version controlled file. And many section in 'pom.xml' is duplicated with 'package.json'.

How about find alternative way to solve the problem without touching package.json and without duplicating the feature in two different places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Leemoonsoo, @1ambda Oh, maybe this was misunderstood. In this context I meant environment which already used UI contributors and their scripts were described in "scripts" in package.json. I Didn't want to broke their usual style of build and test. I have done this only for compatibility with their using of CLI.
@Leemoonsoo Yes duplication Is not good practice
We can get a rid of these "scripts" and run all operations via maven.
e.g. for run build we can run

mvn compile -Dmaven.clean.skip=true

Or if we want to run tests:

mvn test -Dmaven.clean.skip=true

or if we want just create package:

mvn package -Dmaven.clean.skip=true -Dmaven.test.skip=true

Installation of yarn, bower etc. I can put in phase "post-clean". So use -Dmaven.clean.skip=true can save a lot of time for installation of nodejs, yarn and bower
@1ambda @Leemoonsoo Is this variant suit for you?

Copy link
Member

@Leemoonsoo Leemoonsoo Feb 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Current master branch build package with

mvn package

and run test with

mvn test

and it does not install nodejs, yarn, bower every time, unless user run mvn clean.

So, I'm not quite understand what you're trying to achieve here by running command from maven instead of npm script. Or what problem do you have when you run command from npm script instead of maven. Can you elaborate little more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Leemoonsoo which box do you use? Could you give me list of preinstalled packages?
I have used empty headless vagrant images for Centos7 and Ubuntu16 and I have had problem because there were no required packages.
More over at http://zeppelin.apache.org/docs/snapshot/install/build.html#install-requirements
we have list of these required packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Leemoonsoo this is your words:

Like i mentioned above, run 'yarn add xx' changes 'package.json', which is version controlled >file. And many section in 'pom.xml' is duplicated with 'package.json'.

How about find alternative way to solve the problem without touching package.json and >without duplicating the feature in two different places?

I have removed "postinstall" from package.json because it is triggered after each

yarn add

and fail build.
If I run

$ yarn install

from command line this also failed (duplicates for ZEPPELIN-1928)
https://issues.apache.org/jira/browse/ZEPPELIN-1019
https://issues.apache.org/jira/browse/ZEPPELIN-1474
https://issues.apache.org/jira/browse/ZEPPELIN-380

</configuration>
</execution>

<execution>
<id>yarn build</id>
<goals>
<goal>yarn</goal>
</goals>
<configuration>
<arguments>run build</arguments>
</configuration>
<id>yarn karma-phantomjs-launcher</id>
<goals>
<goal>yarn</goal>
</goals>
<phase>process-test-resources</phase>
<configuration>
<arguments>add karma-phantomjs-launcher --no-lockfile --no-bin-links</arguments>
</configuration>
</execution>

<execution>
<id>bower install</id>
<goals>
<goal>bower</goal>
</goals>
<configuration>
<arguments>install --silent --allow-root</arguments>
</configuration>
</execution>

<execution>
<id>yarn test</id>
<goals>
<goal>yarn</goal>
</goals>
<phase>test</phase>
<configuration>
<arguments>run test</arguments>
</configuration>
<id>pre-webpack-dist</id>
<goals>
<goal>grunt</goal>
</goals>
<configuration>
<arguments>pre-webpack-dist</arguments>
</configuration>
</execution>

<execution>
<id>webpack build</id>
<goals>
<goal>webpack</goal>
</goals>
<configuration>
<arguments>-p</arguments>
</configuration>
</execution>

<execution>
<id>post-webpack-dist</id>
<goals>
<goal>grunt</goal>
</goals>
<configuration>
<arguments>post-webpack-dist</arguments>
</configuration>
</execution>

<execution>
<id>javascript tests</id>
<goals>
<goal>karma</goal>
</goals>

<phase>test</phase>
<configuration>
<karmaConfPath>test/karma.conf.js</karmaConfPath>
</configuration>
</execution>

</executions>
Expand Down Expand Up @@ -190,6 +265,15 @@
<fileset>
<directory>node_modules</directory>
</fileset>
<fileset>
<directory>target</directory>
</fileset>
<fileset>
<directory>dist</directory>
</fileset>
<fileset>
<directory>.tmp</directory>
</fileset>
</filesets>
</configuration>
</plugin>
Expand Down
2 changes: 1 addition & 1 deletion zeppelin-web/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ module.exports = function makeWebpackConfig () {

// Output path from the view of the page
// Uses webpack-dev-server in development
publicPath: isProd ? '' : 'http://localhost:9000/',
publicPath: isProd ? '' : './',

// Filename for entry points
// Only adds hash in build mode
Expand Down