feat: add MOZJS_FROM_SOURCE #450

wusyong · 2024-01-24T09:50:07Z

Blocked by #445 and probably need a CI from servo org to publish libmozjs.

This is the last step of #439. Everything should work now but it's based on my fork at the moment.

sagudev · 2024-02-27T05:57:38Z

Maybe we need to automatically enable MOZJS_FROM_SOURCE if features or target are not right but not if MOZJS_ARCHIVE is provided (in this case we trust user that it knows what it is doing).

mozjs-sys/build.rs

wusyong · 2024-03-14T04:34:17Z

The action seems to work on PR and main branch. But I haven't added the job to test auto-downloaded prebuilt. What's preferred job or step to test this?

sagudev · 2024-03-14T09:09:33Z

But I haven't added the job to test auto-downloaded prebuilt. What's preferred job or step to test this?

I think adjusting release.yml to support auto-downloading (instead of manually specifying release/download) would be the way to go. Actually we could always test with auto-download, except when this would require new release (mozjs-sys version was bumped), so in that case we need to fallback to testing artifacts on PR and MQ (but we can auto-download when pushed on main as the release should be made by the same run).

.github/workflows/rust.yml

wusyong · 2024-03-17T12:57:30Z

Behaviors of MOZJS_CREATE_ARCHIVE and MOZJS_ARCHIVE have been updated based on review.
And I added another step in release.yml to auto-download archive and build when it's pushed on main branch.

.github/workflows/release.yml

sagudev · 2024-03-17T13:49:56Z

mozjs-sys/build.rs

+    let build_from_source = if env::var_os("MOZJS_FROM_SOURCE").is_some() || create_archive {
+        true


If I am reading things correctly, if features mismatch with what artifacts provide we run compilation with artifacts anyway and then on failure (if it even fails), we do fallback. I think it would be nicer to fallback to build from source directly. So if streams feature is not selected or debug-mozjs feature is enabled. And when building from source is enabled we should print reason for this using cargo:info for easier debugging.

I added two conditions to check streams and debug-mozjs features. Do they match what you described?

It's good. Is there any specific reason for mixing env::var vs env::var_os, or can we only use one variant everywhere?

Separating MOZJS_FROM_SOURCE and MOZJS_CREATE_ARCHIVE into separate if's with reasons info would also make sense, right?

Otherwise this looks really good, I will do more test after #459 to make sure everything works as expected and then we can land this.

wusyong · 2024-03-22T05:47:42Z

@sagudev Sine #459 has been merged, could this PR be reviewed again? Or should it need to wait for any other PR?

sagudev · 2024-03-22T05:51:37Z

I will do it later today (and if I forgot do not hesitate to ping me tomorrow).

mozjs-sys/build.rs

sagudev

LGTM

But given the nature of this change, maybe this needs another pair of eyes.

mrobinson

Looks pretty good. I just have a few minor comments.

README.md

mozjs-sys/build.rs

mrobinson · 2024-03-26T10:20:30Z

mozjs-sys/build.rs

+    let target = env::var("TARGET").unwrap();
+    let archive_path = PathBuf::from(env::var_os("OUT_DIR").unwrap()).join("libmozjs.tar.gz");
+    if !archive_path.exists() {
+        if !Command::new("curl")


We should document somewhere that cURL is now a dependency.

I added in mozjs's README. I'll also add them to servo's README later.

It would be nice to add this for Windows as well.

Windows has built-in curl as @sagudev mentioned.

mozjs-sys/build.rs

mrobinson · 2024-03-26T10:22:41Z

mozjs-sys/build.rs

+
+/// Link static library tarball instead of building it from source.
+fn link_static_lib_binaries(build_dir: &Path) -> Result<(), std::io::Error> {
+    if let Ok(archive) = env::var("MOZJS_ARCHIVE") {


Could this be mapped to an Option instead of repeating the download_archive and decompress_static_lib calls below?

When it's returned, we will also print the error cause as cargo warning if pre-built archive step fails.
I hope we can keep it as a Result so we can understand why this step failed.

How about something like this:

let archive = let Ok(archive) = env::var("MOZJS_ARCHIVE") { download_archive(Some(&archive)).unwrap_or(PathBuf::from(archive)); } else { download_archive(None)?; }; decompress_static_lib(&archive, build_dir)?;

I believe @sagudev wants decompress_static_lib in the first branch to have a hard fail because it manually specifies MOZJS_ARCHIVE, and we want it to panic instead of fall back.

README.md

mozjs-sys/build.rs

mrobinson · 2024-03-26T13:09:40Z

mozjs-sys/build.rs

+    let target = env::var("TARGET").unwrap();
+    let archive_path = PathBuf::from(env::var_os("OUT_DIR").unwrap()).join("libmozjs.tar.gz");
+    if !archive_path.exists() {
+        if !Command::new("curl")


It would be nice to add this for Windows as well.

mrobinson · 2024-03-26T13:16:59Z

mozjs-sys/build.rs

+
+/// Link static library tarball instead of building it from source.
+fn link_static_lib_binaries(build_dir: &Path) -> Result<(), std::io::Error> {
+    if let Ok(archive) = env::var("MOZJS_ARCHIVE") {


How about something like this:

let archive = let Ok(archive) = env::var("MOZJS_ARCHIVE") { download_archive(Some(&archive)).unwrap_or(PathBuf::from(archive)); } else { download_archive(None)?; }; decompress_static_lib(&archive, build_dir)?;

sagudev · 2024-03-26T13:20:34Z

It would be nice to add this for Windows as well.

curl is preinstalled on windows 10/11.

wusyong · 2024-03-28T11:24:27Z

@sagudev Is it okay to merge this PR?

- If MOZJS_CREATE_ARCHIVE is enabled, it will also build from source. - If MOZJS_ARCHIVE is enabled, it will panic on extraction fail directly.

wusyong mentioned this pull request Feb 13, 2024

feat: add MOZJS_ARCHIVE and MOZJS_CREATE_ARCHIVE env variables #445

Merged

sagudev mentioned this pull request Mar 5, 2024

Add prebuilt mozjs archive CI #454

Merged

wusyong force-pushed the mozjs_download branch from a8e02dc to ea69ebe Compare March 14, 2024 04:08

wusyong commented Mar 14, 2024

View reviewed changes

mozjs-sys/build.rs Outdated Show resolved Hide resolved

sagudev reviewed Mar 14, 2024

View reviewed changes

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

wusyong marked this pull request as ready for review March 17, 2024 13:19

wusyong requested a review from sagudev March 17, 2024 13:20

sagudev reviewed Mar 17, 2024

View reviewed changes

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

sagudev reviewed Mar 17, 2024

View reviewed changes

wusyong requested a review from sagudev March 18, 2024 08:12

sagudev mentioned this pull request Mar 19, 2024

Update SpiderMonkey to ESR115.9 #459

Merged

sagudev reviewed Mar 22, 2024

View reviewed changes

mozjs-sys/build.rs Outdated Show resolved Hide resolved

sagudev mentioned this pull request Mar 22, 2024

Update mozjs to use SpiderMonkey artifacts by default if possible servo/servo#31824

Merged

5 tasks

wusyong requested a review from sagudev March 23, 2024 05:02

sagudev approved these changes Mar 23, 2024

View reviewed changes

sagudev requested a review from mrobinson March 23, 2024 05:25

mrobinson requested changes Mar 26, 2024

View reviewed changes

wusyong requested a review from mrobinson March 26, 2024 12:44

mrobinson reviewed Mar 26, 2024

View reviewed changes

wusyong requested a review from mrobinson March 26, 2024 14:09

mrobinson approved these changes Mar 26, 2024

View reviewed changes

Build from source if archive fails

20bfe5e

wusyong added 11 commits March 28, 2024 12:29

Add MOZJS_FROM_SOURCE

498d7d6

Update env variables behavior

46753bf

- If MOZJS_CREATE_ARCHIVE is enabled, it will also build from source. - If MOZJS_ARCHIVE is enabled, it will panic on extraction fail directly.

Update release job to auto download if release-tag exists

3dbbad5

Update env name in release.yml

e0b6a72

Update prebuilt linking condition

1cd250c

Add more cargo messages during archive linking

ba05231

Update info to normal prints

acc9e39

Cargo fmt

680b142

Update README.md

0341485

Update mozjs-sys build script

c649e64

Update typos and style changes

1648e1c

sagudev force-pushed the mozjs_download branch from c13eb1c to 1648e1c Compare March 28, 2024 11:29

sagudev added this pull request to the merge queue Mar 28, 2024

Merged via the queue into servo:main with commit 8603cbf Mar 28, 2024
20 checks passed

wusyong mentioned this pull request Mar 28, 2024

feat: add a feature flag to build from source #439

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add MOZJS_FROM_SOURCE #450

feat: add MOZJS_FROM_SOURCE #450

wusyong commented Jan 24, 2024

sagudev commented Feb 27, 2024 •

edited

wusyong commented Mar 14, 2024

sagudev commented Mar 14, 2024

wusyong commented Mar 17, 2024

sagudev Mar 17, 2024

wusyong Mar 18, 2024

sagudev Mar 18, 2024

wusyong commented Mar 22, 2024

sagudev commented Mar 22, 2024

sagudev left a comment

mrobinson left a comment

mrobinson Mar 26, 2024

wusyong Mar 26, 2024

mrobinson Mar 26, 2024

wusyong Mar 26, 2024 •

edited

mrobinson Mar 26, 2024

wusyong Mar 26, 2024

mrobinson Mar 26, 2024

wusyong Mar 26, 2024

mrobinson Mar 26, 2024

mrobinson Mar 26, 2024

sagudev commented Mar 26, 2024

wusyong commented Mar 28, 2024

		let build_from_source = if env::var_os("MOZJS_FROM_SOURCE").is_some() \|\| create_archive {
		true

feat: add MOZJS_FROM_SOURCE #450

feat: add MOZJS_FROM_SOURCE #450

Conversation

wusyong commented Jan 24, 2024

sagudev commented Feb 27, 2024 • edited

wusyong commented Mar 14, 2024

sagudev commented Mar 14, 2024

wusyong commented Mar 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusyong commented Mar 22, 2024

sagudev commented Mar 22, 2024

sagudev left a comment

Choose a reason for hiding this comment

mrobinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusyong Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagudev commented Mar 26, 2024

wusyong commented Mar 28, 2024

sagudev commented Feb 27, 2024 •

edited

wusyong Mar 26, 2024 •

edited