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

#10881 migrate from bower to yarn #10883

Closed
wants to merge 1 commit into from

Conversation

nickboldt
Copy link
Contributor

#10881 WIP to migrate from bower to yarn.

Change-Id: Ic93a1191bcb25fdc564b481bc5c5877fc233a617
Signed-off-by: nickboldt [email protected]

@riuvshin
Copy link
Contributor

Can one of the admins verify this patch?

2 similar comments
@riuvshin
Copy link
Contributor

Can one of the admins verify this patch?

@riuvshin
Copy link
Contributor

Can one of the admins verify this patch?

@nickboldt
Copy link
Contributor Author

nickboldt commented Aug 22, 2018

To builds this, I ran:

git clone [email protected]:eclipse/che.git
cd che/dashboard
export TMPDIR=/tmp
mvn clean install -Pnative -V -e -Pfast,native -Dskip-enforce -DskipTests \
-Dskip-validate-sources -Dfindbugs.skip -DskipIntegrationTests=true -Dmdep.analyze.skip=true \
-Dmaven.javadoc.skip -Dgpg.skip -Dorg.slf4j.simpleLogger.showDateTime=true \
-Dorg.slf4j.simpleLogger.dateTimeFormat=HH:mm:ss \
-Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn \
| tee ../../log.dashboard-only.`date +%s`.txt

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

Bower dependencies can't become devDependencies. They're dependencies. This PR is mixing build and runtime dependencies

@sleshchenko
Copy link
Member

According to PR description template https://raw.githubusercontent.com/eclipse/che/master/.github/PULL_REQUEST_TEMPLATE.md

<!-- #### Changelog -->
<!-- The changelog will be pulled from the PR's title. 
     Please provide a clear and meaningful title to the PR and don't include issue number -->

So, please fix PR title before merging.
Thanks.

@nickboldt nickboldt changed the title https://github.com/eclipse/che/issues/10881... #10881 migrate from bower to yarn Aug 23, 2018
@nickboldt
Copy link
Contributor Author

nickboldt commented Aug 23, 2018

PR title fixed, @sleshchenko .

Changes requested by @benoitf: move devDeps to prod deps.

I also:

  • migrated to bootstrap-styl (replaces bootstrap-stylus)
  • made mvn clean delete yarn.lock
  • removed ref to tty.isatty

Note that this works with node 6.14.4 and npm 3.10.10. When I tried with node 8.11.3 / npm 5.6, the node-gyp failures were recorded as failures, rather than optional warnings, so the build fails.

I've not managed to figure out how to tell yarn to ignore node-gyp errors yet.

It's also been suggested that we look at migrating from gulp & browserify to webpack, but I think that can be done in a second PR, once this is approved.

Can anyone verify this works when you build it locally, etc? Compiles for me but I've no idea what to then run to verify everything LOOKS the same / RUNS the same / WORKS the same. Is there a smoke test doc somewhere I can read?

move devDeps to prod deps; migrate to bootstrap-styl (replaces bootstrap-stylus); make mvn clean delete yarn.lock; remove ref to tty.isatty

Change-Id: Ic93a1191bcb25fdc564b481bc5c5877fc233a617
Signed-off-by: nickboldt <[email protected]>
@nickboldt
Copy link
Contributor Author

Applying this patch is getting me further in NCL. Can someone have a look and merge it (or something similar, if you don't like all my changes) in time for the 6.11 release?

If so, that would be amazing! If not, please let me know why this is rejected or when it can be done in future.

cc: @ashumilova @sleshchenko @benoitf

@ashumilova
Copy link
Contributor

@nickboldt
Tested the PR locally: for me yarn install is not working (fails with gyp ERR! build error) - tried with same node and npm versions, that you use.
When tried to run the application locally with gulp (command: gulp serve --server=http://172.19.20.179:8080) : also having errors
screenshot from 2018-08-27 10-31-12

And there is still bower build in Dockerfile (in the root of dashboard directory).

@nickboldt
Copy link
Contributor Author

nickboldt commented Aug 27, 2018

@ashumilova I wasn't going to change the dockerfile until I had a working local build (-Pnative). I'm not using the docker profile as it doesn't work in any internal RH systems I'm using (Brew, NCL).

Can you npm install yarn in the dashboard folder before running mvn clean install -Pnative -DskipTests ... ? does that fix the error?

@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Aug 28, 2018
@nickboldt
Copy link
Contributor Author

replaced by another PR

@nickboldt nickboldt closed this Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants