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

cpu: fix building for js #2

Merged
merged 1 commit into from
Mar 21, 2022
Merged

cpu: fix building for js #2

merged 1 commit into from
Mar 21, 2022

Conversation

paralin
Copy link
Contributor

@paralin paralin commented Mar 21, 2022

Avoid importing the x/sys/cpu package for the js build tag.

Mark that CPU target has having none of the CPU features.

Fixes the build against gopherjs.

@marten-seemann
Copy link
Owner

  1. What's the use case for this?
  2. Why are you exposing these as variables, why not unexported constants?
  3. We'd also need to make the equivalent changes to https://github.com/marten-seemann/qtls-go1-16 and https://github.com/marten-seemann/qtls-go1-18, unfortunately.

egonelbre
egonelbre reviewed Mar 21, 2022
View reviewed changes
@paralin paralin mentioned this pull request Mar 21, 2022
Merged
@paralin
Copy link
Contributor Author

paralin commented Mar 21, 2022 •
edited
Loading

image

  1. I run Quic in the browser over WebSocket and other underlying transports using either GopherJS or Wasm depending on what support is available.
  2. They can be unexported, sure.
  3. I'll link updated PRs for 1-16 and 1-17 and will update this one to unexport the variables.

@paralin
cpu: fix building for js
97b65fe 
Avoid importing the x/sys/cpu package for the js build tag.

Mark that CPU target has having none of the CPU features.

Fixes the build against gopherjs.

Signed-off-by: Christian Stewart <christian@paral.in>
@paralin
Copy link
Contributor Author

paralin commented Mar 21, 2022

Equiv. patches for the other versions:

Unexported the variables & tested with all 3 versions

@marten-seemann
Copy link
Owner

marten-seemann/qtls-go1-15#2

We need https://github.com/marten-seemann/qtls-go1-18, not https://github.com/marten-seemann/qtls-go1-15. Sorry for this mess with the qtls versions btw ;)

@paralin
Copy link
Contributor Author

paralin commented Mar 21, 2022 •
edited
Loading

quic-go/qtls-go1-18#2

It's a bit of a mess, but I understand why it's necessary.

@marten-seemann marten-seemann merged commit 17223cd into marten-seemann:master Mar 21, 2022
marten-seemann pushed a commit that referenced this pull request Jun 5, 2022
@paralin @marten-seemann
cpu: fix building for js (#2)
5a78e07 
Avoid importing the x/sys/cpu package for the js build tag.

Mark that CPU target has having none of the CPU features.

Fixes the build against gopherjs.

Signed-off-by: Christian Stewart <christian@paral.in>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egonelbre egonelbre egonelbre left review comments

@marten-seemann marten-seemann marten-seemann approved these changes

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
@paralin @marten-seemann @egonelbre