Skip to content

/ servo

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

Jump to bottom

dependencies required for mozjs #20888

Closed
tigercosmos opened this issue May 31, 2018 · 15 comments
Closed

dependencies required for mozjs #20888

tigercosmos opened this issue May 31, 2018 · 15 comments
Labels
A-build

Comments

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented May 31, 2018 •
edited

since recent commits, the build will fail with C++ errors. Either in Win10, or Ubuntu 16.04, as far as I know.

I add apt install clang and fix the Ubuntu. But I don't know how to fix Windows.
@jdm jdm added the A-build label May 31, 2018
@kwonoj
Copy link
Contributor

@kwonoj kwonoj commented May 31, 2018 •
edited

This seems related to mozjs now requires clang. For windows, easiest way is install llvm as instructed in mozjs (https://github.com/servo/mozjs/#building)
@kwonoj
Copy link
Contributor

@kwonoj kwonoj commented May 31, 2018

actually it isn't 100% true, I got some other failures in mozjs still. need dig bit more.
@tigercosmos
Copy link
Collaborator Author

@tigercosmos tigercosmos commented Jun 1, 2018

d0cc9d2 did add lib in script. I don't know why it doesn't work.
@tigercosmos tigercosmos changed the title dependencies require clang dependencies required for mozjs Jun 1, 2018
@kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Jun 1, 2018

@tigercosmos actually that reveals reason why for me on windows, d0cc9d2#diff-e6bf833f91496efdbd7e4e9ecfc7a4cdR7 points deps to llvm 4 while mozjs requires 6. I tried manually handcraft script to point local installation to llvm 6, and was able to get build working.

Guess resolution'll be update deps to 6, while servo-deps.s3.amazonaws.com/msvc-deps doesn't have it updated yet?
@tigercosmos
Copy link
Collaborator Author

@tigercosmos tigercosmos commented Jun 1, 2018

@kwonoj sounds reasonable. What do you think @asajeffrey?
@tigercosmos
Copy link
Collaborator Author

@tigercosmos tigercosmos commented Jun 1, 2018 •
edited

@jdm Could we put the file llvm-6.0.0.zip to https://servo-deps.s3.amazonaws.com/msvc-deps/?
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 1, 2018

Hmm, mozjs requires llvm 6? I thought it only needed llvm 4.

We could upload llvm6 to aws, but I'm a bit concerned this might break anything that depends on old versions of clang (I seem to remember osmesa being picky?)
@tigercosmos
Copy link
Collaborator Author

@tigercosmos tigercosmos commented Jun 1, 2018

@asajeffrey could let both llvm 4 and llvm 6 on the cloud. Servo read the version in package.py. We could test it first.
@tigercosmos tigercosmos mentioned this issue Jun 1, 2018
Merged
0 of 5 tasks complete
bors-servo added a commit that referenced this issue Jun 1, 2018
@bors-servo
Auto merge of #20894 - tigercosmos:rrr, r=avadacatavra
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
c3dee88 
readme add llvm clang

<!-- Please describe your changes on the following line: -->

update readme, since recent commit required the dependencies.
part of #20888

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20894)
<!-- Reviewable:end -->
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 1, 2018

Could do but I'm a bit surprised llvm 6 is needed, llvm 4 works for me on my local machine, and also on the build machines.
@kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Jun 1, 2018

may be related to msvc version? this is message I got on local machines for reference.

cargo:rustc-link-search=native=C:\github\servo\target\debug\build\mozjs_sys-e622a19cc3f27e25\out
TARGET = Some("x86_64-pc-windows-msvc")
Generting bindings ["./src/jsglue.hpp", "--rust-target", "1.19", "--rustified-enum", ".*", "--no-derive-default", "--enable-cxx-namespaces", "--generate", "function,types,vars", "--ignore-methods", "--", "-I", "C:\\github\\servo\\target\\debug\\build\\mozjs_sys-e622a19cc3f27e25\\out\\dist/include", "-x", "c++", "-fms-compatibility", "-DRUST_BINDGEN", "-DSTATIC_JS_API", "-std=c++14", "-DWIN32"].

--- stderr
C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.14.26428\include\yvals.h:297:5: error: STL1000: Unexpected compiler version, expected Clang 6 or newer.
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 1, 2018

Ah, this is a check MS introduced to ensure that the toolchain is at least as recent as the stl. https://blogs.msdn.microsoft.com/vcblog/2017/12/19/c17-progress-in-vs-2017-15-5-and-15-6/

Sigh, seems like we need to use clang 6 if we want to support VS 2017.

I'll upload llvm-6.0.0.zip then we can experiment with using it.
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 1, 2018

OK, I uploaded https://servo-deps.s3.amazonaws.com/msvc-deps/llvm-6.0.0.zip
@kwonoj kwonoj mentioned this issue Jun 1, 2018
Merged
1 of 5 tasks complete
@kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Jun 1, 2018 •
edited

Created one-liner change #20895 to bump. (mostly to see if CI passes other than windows)
bors-servo added a commit that referenced this issue Jun 1, 2018
@bors-servo
Auto merge of #20895 - kwonoj:bump-dep, r=asajeffrey
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
0c703fb 
build(servo): bump up llvm to 6.0.0

<!-- Please describe your changes on the following line: -->
This PR bumps up llvm to 6.0.0 to allow vs2017 build works on windows.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #20888 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20895)
<!-- Reviewable:end -->
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 1, 2018

OK, lets see if that works on CI.
@tigercosmos tigercosmos reopened this Jun 2, 2018
@tigercosmos
Copy link
Collaborator Author

@tigercosmos tigercosmos commented Jun 2, 2018

Sorry. My fault.
@tigercosmos tigercosmos closed this Jun 2, 2018
@jdm jdm mentioned this issue Jan 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-build
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

build(servo): bump up llvm to 6.0.0
4 participants
@jdm @asajeffrey @kwonoj @tigercosmos
You can’t perform that action at this time.