Use moztools instead of mozilla-build #368

sagudev · 2023-08-17T08:32:00Z

This makes mozjs on windows more standalone and allows to remove presetting variables in servo.

Servo's ./mach bootstrap can still download moztools and mozjs will use downloaded moztools if they are right version.

WIP because I still need to make companion PR for servo.

Fixes #300
Supersedes #326 & #356

mrobinson · 2023-08-17T08:44:49Z

This is a nice change!

Some suggestions:

Can you have build.rs not download anything yet, but focus on whether or not to do that in a followup? Just download Servo's Mozilla tools in the GitHub action. That way we have two clean changes:
- Switch to Servo's packaged tools.
- Decide if we want build.rs to take care of that.
The documentation should be updated to reflect this change.
Let's move the changes to the actions unrelated to this change to a new PR (not running actions in forks).

sagudev · 2023-08-17T08:55:07Z

This could be problematic as if we switch to moztools but do not implement downloading step the in this middle commit users needs to manually set MOZ_TOOLS_PATH or manually download moztools and extract it to target/dependencies, so I think it would be better to have it in same change.
Will do!
Run CI only on master in upstream but everywhere in forks #369

mrobinson · 2023-08-17T09:09:31Z

This could be problematic as if we switch to moztools but do not implement downloading step the in this middle commit users needs to manually set MOZ_TOOLS_PATH or manually download moztools and extract it to target/dependencies, so I think it would be better to have it in same change.

This is the status quo with MozillaBuild so I think it's fine to keep this behavior for now. I think we should hold off on the download step...because we really haven't decided what to do with this dependency. For instance, maybe we could convert it into a crate and avoid having to do a manual download at all.

sagudev · 2023-08-17T09:16:43Z

This could be problematic as if we switch to moztools but do not implement downloading step the in this middle commit users needs to manually set MOZ_TOOLS_PATH or manually download moztools and extract it to target/dependencies, so I think it would be better to have it in same change.

This is the status quo with MozillaBuild so I think it's fine to keep this behavior for now. I think we should hold off on the download step...because we really haven't decided what to do with this dependency. For instance, maybe we could convert it into a crate and avoid having to do a manual download at all.

Using mozilla-build was easier as you can just simply install mozilla-build using installer and open mozilla-build shell (like you would do if you wanted to develop for firefox) and run commands from there. Manually setting MOZILLA_BUILD was just a trick to simulate mozilla-build env in github actions where you cannot open mozilla-build shell to run commands.

mozjs/build.rs

mrobinson

I think we should not download moztools in build.rs. In addition the comments above, this introduces a hidden dependency on cURL to build this crate. Let's just do the minimal change here and require that it be unzipped ahead of time and an environment variable set.

sagudev · 2023-08-17T13:16:52Z

In addition the comments above, this introduces a hidden dependency on cURL to build this crate.

Curl is installed on every Win10+ system by default otherwise we could just depend on some rust crate for downloading.

I already have separation per commits, but now I would like to test with servo first and then do changes here.

sagudev · 2023-08-18T08:27:08Z

Successful servo test here: https://github.com/sagudev/servo/actions/runs/5899499296

Redfire75369 · 2023-08-18T16:09:03Z

I think this should accept either MOZ_TOOLS_PATH or MOZILLA_BUILD aa the env variable for backwards compatibility. Mainly outside of servo, this could be pretty annoying as a change.

My suggestion is env::var_os("MOZ_TOOLS_PATH").or_else(|| env::var_os("MOZILLA_BUILD").

sagudev · 2023-08-18T17:11:25Z

I think this should accept either MOZ_TOOLS_PATH or MOZILLA_BUILD aa the env variable for backwards compatibility. Mainly outside of servo, this could be pretty annoying as a change.

You are right, while we are still on moztools-3.2 we can still support MozillaBuild 3.2+

sagudev · 2023-08-25T10:21:51Z

@mrobinson any progress?

mrobinson

Sorry for the delay in reviewing. This looks good to me. The one larger change I think we should have apart from the small things below is that we should update the README to explain downloading, unzipping, and setting the environment variable for moztools. Thanks!

mozjs/build.rs

.github/workflows/rust.yml

mozjs/build.rs

mrobinson

Sorry, a couple other tiny things.

mozjs/build.rs

mrobinson · 2023-08-31T10:29:20Z

mozjs/src/jsimpls.rs

-        ObjectOpResult {
-            code_: ObjectOpResult_SpecialCodes::Uninitialized as usize,
-        }
+        ObjectOpResult { code_: ObjectOpResult_SpecialCodes::Uninitialized as usize }


Were these changes added by mistake?

I run cargo fmt and I guess this was not formatted properly before.

mrobinson

Looks good to me, thanks!

sagudev · 2023-08-31T11:09:49Z

Failures are not my fault: actions/runner-images#8125

sagudev force-pushed the moztool branch from 22ab2e4 to 540fec7 Compare August 17, 2023 09:06

sagudev force-pushed the moztool branch 2 times, most recently from 9acc49f to 2882c4a Compare August 17, 2023 10:54

Redfire75369 suggested changes Aug 17, 2023

View reviewed changes

sagudev force-pushed the moztool branch 2 times, most recently from 288ac5e to cece0e2 Compare August 17, 2023 11:19

sagudev requested a review from Redfire75369 August 17, 2023 11:20

Redfire75369 suggested changes Aug 17, 2023

View reviewed changes

mozjs/build.rs Outdated Show resolved Hide resolved

mozjs/build.rs Outdated Show resolved Hide resolved

sagudev force-pushed the moztool branch from cece0e2 to bc714a5 Compare August 17, 2023 11:29

sagudev requested a review from Redfire75369 August 17, 2023 11:30

Redfire75369 approved these changes Aug 17, 2023

View reviewed changes

mrobinson requested changes Aug 17, 2023

View reviewed changes

sagudev mentioned this pull request Aug 17, 2023

Source of dist files servo/servo-build-deps#1

Closed

sagudev force-pushed the moztool branch 7 times, most recently from 00bf60e to 2cbad03 Compare August 18, 2023 06:13

sagudev marked this pull request as ready for review August 18, 2023 08:27

sagudev requested a review from mrobinson August 18, 2023 08:27

Use moztools instead of mozilla-build

49d791c

sagudev force-pushed the moztool branch from 2cbad03 to 49d791c Compare August 18, 2023 17:08

mrobinson requested changes Aug 31, 2023

View reviewed changes

mozjs/build.rs Outdated Show resolved Hide resolved

mozjs/build.rs Outdated Show resolved Hide resolved

.github/workflows/rust.yml Outdated Show resolved Hide resolved

mozjs/build.rs Outdated Show resolved Hide resolved

mrobinson requested changes Aug 31, 2023

View reviewed changes

mozjs/build.rs Outdated Show resolved Hide resolved

mozjs/build.rs Outdated Show resolved Hide resolved

sagudev force-pushed the moztool branch from b137c38 to bc405b5 Compare August 31, 2023 10:27

mrobinson reviewed Aug 31, 2023

View reviewed changes

mrobinson approved these changes Aug 31, 2023

View reviewed changes

mrobinson enabled auto-merge August 31, 2023 10:34

Document moztools

f1c2d72

auto-merge was automatically disabled August 31, 2023 10:45
Head branch was pushed to by a user without write access

sagudev force-pushed the moztool branch from bc405b5 to f1c2d72 Compare August 31, 2023 10:45

sagudev mentioned this pull request Sep 4, 2023

Normal formatting & fixes generate_wrappers.sh #379

Merged

mukilan mentioned this pull request Sep 4, 2023

use windows-2019 until build with windows-2022 is fixed #380

Merged

Merge branch 'master' into moztool

a47d215

sagudev requested a review from mrobinson September 4, 2023 16:15

mrobinson approved these changes Sep 4, 2023

View reviewed changes

mrobinson enabled auto-merge September 4, 2023 16:20

mrobinson added this pull request to the merge queue Sep 4, 2023

Merged via the queue into servo:master with commit 8dcf64e Sep 4, 2023
12 checks passed

sagudev mentioned this pull request Mar 8, 2024

failed to run custom build command for mozjs_sys v0.68.2 (https://github.com/servo/mozjs#98b67828) servo/servo#30008

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use moztools instead of mozilla-build #368

Use moztools instead of mozilla-build #368

sagudev commented Aug 17, 2023 •

edited

Loading

mrobinson commented Aug 17, 2023 •

edited

Loading

sagudev commented Aug 17, 2023 •

edited by mrobinson

Loading

mrobinson commented Aug 17, 2023

sagudev commented Aug 17, 2023

mrobinson left a comment

sagudev commented Aug 17, 2023 •

edited

Loading

sagudev commented Aug 18, 2023

Redfire75369 commented Aug 18, 2023 •

edited

Loading

sagudev commented Aug 18, 2023

sagudev commented Aug 25, 2023

mrobinson left a comment

mrobinson left a comment

mrobinson Aug 31, 2023

sagudev Aug 31, 2023

mrobinson left a comment

sagudev commented Aug 31, 2023

Use moztools instead of mozilla-build #368

Use moztools instead of mozilla-build #368

Conversation

sagudev commented Aug 17, 2023 • edited Loading

mrobinson commented Aug 17, 2023 • edited Loading

sagudev commented Aug 17, 2023 • edited by mrobinson Loading

mrobinson commented Aug 17, 2023

sagudev commented Aug 17, 2023

mrobinson left a comment

Choose a reason for hiding this comment

sagudev commented Aug 17, 2023 • edited Loading

sagudev commented Aug 18, 2023

Redfire75369 commented Aug 18, 2023 • edited Loading

sagudev commented Aug 18, 2023

sagudev commented Aug 25, 2023

mrobinson left a comment

Choose a reason for hiding this comment

mrobinson left a comment

Choose a reason for hiding this comment

mrobinson Aug 31, 2023

Choose a reason for hiding this comment

sagudev Aug 31, 2023

Choose a reason for hiding this comment

mrobinson left a comment

Choose a reason for hiding this comment

sagudev commented Aug 31, 2023

sagudev commented Aug 17, 2023 •

edited

Loading

mrobinson commented Aug 17, 2023 •

edited

Loading

sagudev commented Aug 17, 2023 •

edited by mrobinson

Loading

sagudev commented Aug 17, 2023 •

edited

Loading

Redfire75369 commented Aug 18, 2023 •

edited

Loading