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

Fix binary selection #476

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Conversation

samoylovfp
Copy link
Contributor

@samoylovfp samoylovfp commented Jan 4, 2023

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).
  • Updated site content with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will 🤓.

I don't think updating README or site is warranted here.
@thedodd please squash the commits if you think it is beneficial

src/pipelines/rust.rs Outdated Show resolved Hide resolved
@samoylovfp
Copy link
Contributor Author

samoylovfp commented Jan 4, 2023

Example of the error triggering:
image

src/pipelines/rust.rs Outdated Show resolved Hide resolved
src/pipelines/rust.rs Outdated Show resolved Hide resolved
@samoylovfp samoylovfp force-pushed the fix-binary-selection branch from 56b5d0b to 7b6ddc3 Compare January 4, 2023 17:13
@samoylovfp samoylovfp requested a review from simbleau January 4, 2023 19:02
@samoylovfp samoylovfp force-pushed the fix-binary-selection branch 2 times, most recently from 44c4305 to e17f331 Compare January 5, 2023 10:50
@samoylovfp
Copy link
Contributor Author

Is anything still required from me to get this merged?

@thedodd
Copy link
Member

thedodd commented Mar 23, 2023

Looks great! @samoylovfp I'm not sure why CI is not triggering. Do you mind rebasing your branch? Hopefully that will allow CI to be triggered.

@thedodd thedodd added the disposition:merge The tagged item should probably be merged label Mar 23, 2023
@samoylovfp
Copy link
Contributor Author

samoylovfp commented Mar 23, 2023 via email

@samoylovfp samoylovfp force-pushed the fix-binary-selection branch from e17f331 to 94e9611 Compare March 26, 2023 09:17
@samoylovfp
Copy link
Contributor Author

Really stretching the definition of a "couple", but here we are

@samoylovfp
Copy link
Contributor Author

Planning to fix the tests during the next weekend

@thedodd
Copy link
Member

thedodd commented Mar 29, 2023

@samoylovfp as it turns out, at least one of the test failures (building the webworker-gloo example) demonstrates that your changeset does what it says on the tin:

found more than one binary crate: ["webworker-gloo", "worker"], consider adding `<link data-trunk rel="rust" data-bin={bin} />` to the index.html

@samoylovfp samoylovfp force-pushed the fix-binary-selection branch from 94e9611 to 10e947c Compare April 1, 2023 11:56
@samoylovfp
Copy link
Contributor Author

samoylovfp commented Apr 1, 2023

Whew, good thing the test was there!

Here's how I understood the issue:
webworker-gloo example is working fine in the main branch because cargo always first builds the library and then the binary and the master version of trunk picks up the last of artifacts returned by cargo. Now that cargo_build checks the total number, it started failing.

So I made cargo_build filter out only the binary artifacts for the validation and for choosing which one to use as the wasm entrypoint.

proceeding leads to indeterminte result
where trunk picks up an arbitrary binary for the target.
See trunk-rs#475
@samoylovfp samoylovfp force-pushed the fix-binary-selection branch from 10e947c to 1ae3b77 Compare April 3, 2023 15:07
@thedodd
Copy link
Member

thedodd commented Apr 24, 2023

Gonna merge this after CI goes green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition:merge The tagged item should probably be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants