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

Use github artifact attestations for the prebuilt artifacts #503

Merged
merged 7 commits into from
Nov 1, 2024

Conversation

jschwe
Copy link
Member

@jschwe jschwe commented Sep 10, 2024

I tested this on my local fork (changed line 908 to point to jschwe/mozjs.
On my work machine the attestation takes around 500ms, but network is generally slow (downloading the 16MB artifact via curl takes somewhere between 20 and 60s for some reason)

@jschwe jschwe force-pushed the jschwender/use_prebuilts_ohos branch from 8155c1c to 797c728 Compare September 10, 2024 13:12
Copy link
Member

@mukilan mukilan left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I guess someone more familiar with the prebuilt artifact feature can also review. cc @sagudev, @wusyong

@jschwe
Copy link
Member Author

jschwe commented Sep 24, 2024

I would love to see this merged, simply since it would allow servo builds for OH to work on windows. If there are objections to using gh to verify the OH shared library, then I can also remove that for now.

@sagudev
Copy link
Member

sagudev commented Sep 24, 2024

Shouldn't prebuilt artifacts already work?

@jschwe
Copy link
Member Author

jschwe commented Sep 24, 2024

true, I guess the issue is that we never cut a release after #501. Should i open a seperate PR which just bumps the version?

@sagudev
Copy link
Member

sagudev commented Sep 24, 2024

Ah we didn't bump mozjs-sys ver in #501, so that's why there is no ohos release. So what this PR actually does is use attestation for ohos artifact. If we decide to introduce attestation, we should introduce it for all targets.

@sagudev
Copy link
Member

sagudev commented Sep 24, 2024

Should i open a seperate PR which just bumps the version?

That would be ideal.

@jschwe
Copy link
Member Author

jschwe commented Sep 24, 2024

If we decide to introduce attestation, we should introduce it for all targets.

I remember someone on zulip had reservations about potential influence on runtime of the build-script, so I thought perhaps we should first get some real-world experience on one target, before affecting the commonly used ones

@sagudev
Copy link
Member

sagudev commented Sep 24, 2024

I remember someone on zulip had reservations about potential influence on runtime of the build-script, so I thought perhaps we should first get some real-world experience on one target, before affecting the commonly used ones

That was me :). I think we should make make it for all platforms but let it be disabled by default (enabled via MOZJS_ATTESTATION), this way we can test it reasonably faster and then decide.

@jschwe jschwe changed the title Use prebuilt artifact for the openharmony build Use github artifact attestations for the prebuilt artifacts Sep 24, 2024
@jschwe jschwe force-pushed the jschwender/use_prebuilts_ohos branch from 797c728 to e87091e Compare September 25, 2024 05:11
@jschwe
Copy link
Member Author

jschwe commented Sep 25, 2024

MOZJS_ATTESTATION

How fancy do we want to make this switch? I guess having an option like MOZJS_ATTESTATION=force, which causes the build-script to exit with an error if gh is not found would be nice. I personally would say we could just differentiate between the variants "force", "variable not set" and "set to something !=force" and wouldn't bother with checking for values like false, off, no, 0 etc. What do you think?

@jschwe jschwe marked this pull request as draft September 28, 2024 13:10
@sagudev
Copy link
Member

sagudev commented Sep 29, 2024

MOZJS_ATTESTATION

How fancy do we want to make this switch? I guess having an option like MOZJS_ATTESTATION=force, which causes the build-script to exit with an error if gh is not found would be nice. I personally would say we could just differentiate between the variants "force", "variable not set" and "set to something !=force" and wouldn't bother with checking for values like false, off, no, 0 etc. What do you think?

Sounds good.

@jschwe jschwe force-pushed the jschwender/use_prebuilts_ohos branch 2 times, most recently from 39ef251 to 31af738 Compare October 2, 2024 07:05
@jschwe
Copy link
Member Author

jschwe commented Oct 2, 2024

Is it possible we need to update the upstream mozjs COMMIT again? seems like the artifact download is failing.

@jschwe jschwe marked this pull request as ready for review October 2, 2024 07:37
@sagudev
Copy link
Member

sagudev commented Oct 2, 2024

Is it possible we need to update the upstream mozjs COMMIT again? seems like the artifact download is failing.

We need to update SM to new minor release. I will do it later today.

@jschwe jschwe force-pushed the jschwender/use_prebuilts_ohos branch 4 times, most recently from 38beed5 to 6ee294f Compare October 7, 2024 11:14
@jschwe jschwe requested review from mukilan and sagudev October 7, 2024 11:54
@sagudev
Copy link
Member

sagudev commented Oct 8, 2024

I will review this at the end of the week.

Copy link
Member

@sagudev sagudev left a comment

Choose a reason for hiding this comment

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

Generally LGTM, still need to test this on my machine.

mozjs-sys/build.rs Outdated Show resolved Hide resolved
mozjs-sys/build.rs Outdated Show resolved Hide resolved
@jschwe jschwe force-pushed the jschwender/use_prebuilts_ohos branch 2 times, most recently from b018d2d to 39cbe9c Compare October 26, 2024 04:37
@jschwe jschwe force-pushed the jschwender/use_prebuilts_ohos branch 3 times, most recently from 8b3e9fb to 9a8e93f Compare October 30, 2024 09:40
jschwe and others added 7 commits November 1, 2024 14:29
Signed-off-by: Jonathan Schwender <[email protected]>

f

Signed-off-by: Jonathan Schwender <[email protected]>
Signed-off-by: Jonathan Schwender <[email protected]>
Signed-off-by: Jonathan Schwender <[email protected]>
Signed-off-by: Jonathan Schwender <[email protected]>
Signed-off-by: Jonathan Schwender <[email protected]>
@jschwe jschwe force-pushed the jschwender/use_prebuilts_ohos branch from 9a8e93f to 65e2939 Compare November 1, 2024 13:32
@jschwe jschwe requested a review from sagudev November 1, 2024 13:32
@mrobinson
Copy link
Member

Seems good to me. Let's give this a shot.

@mrobinson mrobinson added this pull request to the merge queue Nov 1, 2024
Merged via the queue into servo:main with commit 3a0a5f0 Nov 1, 2024
27 checks passed
@jschwe jschwe deleted the jschwender/use_prebuilts_ohos branch November 20, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants