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.

Sign up for GitHub

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

chore: update deno_file to use deno_webidl #10042

Merged
merged 4 commits into from
Apr 7, 2021
Merged

chore: update deno_file to use deno_webidl #10042

merged 4 commits into from
Apr 7, 2021

Conversation

lucacasonato
Copy link
Member

This changes the custom input converters in deno_file to use deno_webidl
converters.

@lucacasonato lucacasonato mentioned this pull request Apr 6, 2021
Merged
@lucacasonato
Copy link
Member Author

Unsure why mac CI is failing... builds fine on M1. @bartlomieju could you try to build this locally?

kitsonk
kitsonk reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

I am getting build failures locally on my Intel mac. Not sure why.

op_crates/file/01_file.js
@@ -300,42 +344,62 @@
/**
* @param {BlobPart[]} fileBits
* @param {string} fileName
* @param {FilePropertyBag} [options]
* @param {FilePropertyBag} options
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param {FilePropertyBag} options
* @param {FilePropertyBag=} options

Copy link
Member Author

@lucacasonato lucacasonato Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Results in options is possibly undefined, which is wrong.

@bartlomieju
Copy link
Member

I'm getting a segfault when building on mac:

signal: 11, SIGSEGV: invalid memory reference

@lucacasonato
Copy link
Member Author

Kitson was getting that too. No idea where it is coming from. I can't reproduce on M1 or Linux. Is it segfaulting during snapshotting?

@bartlomieju
Copy link
Member

Kitson was getting that too. No idea where it is coming from. I can't reproduce on M1 or Linux. Is it segfaulting during snapshotting?

I think it is coming from snapshot:

   Compiling deno_runtime v0.10.1 (/Users/biwanczuk/dev/deno/runtime)
       Fresh swc_ecmascript v0.24.1
       Fresh swc_bundler v0.25.1
     Running `/Users/biwanczuk/dev/deno/target/debug/build/deno_runtime-5f47a5fb67edbe5c/build-script-build`
       Fresh dprint-swc-ecma-ast-view v0.10.0
       Fresh deno_doc v0.1.23
       Fresh deno_lint v0.2.19
       Fresh dprint-plugin-typescript v0.41.0
[deno_runtime 0.10.1] cargo:rustc-env=TARGET=x86_64-apple-darwin
[deno_runtime 0.10.1] cargo:rustc-env=PROFILE=debug
error: failed to run custom build command for `deno_runtime v0.10.1 (/Users/biwanczuk/dev/deno/runtime)`

Caused by:
  process didn't exit successfully: `/Users/biwanczuk/dev/deno/target/debug/build/deno_runtime-5f47a5fb67edbe5c/build-script-build` (signal: 11, SIGSEGV: invalid memory reference)
  --- stdout
  cargo:rustc-env=TARGET=x86_64-apple-darwin
  cargo:rustc-env=PROFILE=debug

@kitsonk
Copy link
Contributor

kitsonk commented Apr 7, 2021

A bit of experimenting and it is this line: https://github.com/denoland/deno/pull/10042/files#diff-4e14bca696f63fff1ff474012a00ccefd7f92183d547304e97c3fc2af5c2cfdeR624

It is touching .locale during snapshotting, which is touching ICU, which we believe has duplicate symbols because of the WebGPU stuff, which on non-M1 macs we know is a problem on debug builds.

@lucacasonato lucacasonato force-pushed the blob_webidl branch 2 times, most recently from 3141dcc to e80c18b Compare April 7, 2021 14:16
@lucacasonato
chore: update deno_file to use deno_webidl
65e186b 
This changes the custom input converters in deno_file to use deno_webidl
converters.
@lucacasonato
don't use localeCompare
c2a1663
@lucacasonato
add webidl for blob url
1d07fb4
kitsonk
kitsonk approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

LGTM

@lucacasonato lucacasonato merged commit ee07ef2 into denoland:main Apr 7, 2021
@lucacasonato lucacasonato deleted the blob_webidl branch April 7, 2021 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kitsonk kitsonk kitsonk approved these changes

@bartlomieju bartlomieju Awaiting requested review from bartlomieju

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lucacasonato @bartlomieju @kitsonk