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

Fix "can't leak crate-private type in stub.rs" #662 #684

Merged
merged 7 commits into from
May 12, 2022
Merged

Fix "can't leak crate-private type in stub.rs" #662 #684

merged 7 commits into from
May 12, 2022

Conversation

AmateurECE
Copy link
Contributor

First attempt to fix this issue for wasm32-unknown-* targets by
changing visibility of functions not to leak crate-private types.

Tested and it builds correctly for wasm32-unknown-unknown.

Thanks for contributing to chrono!

  • Have you added yourself and the change to the changelog? (Don't worry
    about adding the PR number)
  • If this pull request fixes a bug, does it add a test that verifies that
    we can't reintroduce it?

@AmateurECE
Can't leak crate-private type in stub.rs #662
1663f45 
First attempt to fix this issue for wasm32-unknown-* targets by
changing visibility of functions not to leak crate-private types.
@djc
Copy link
Member

djc commented May 1, 2022

So have you got any suggestions for testing this in CI? I'm worried that this will just break again if it's untested.

@AmateurECE
Copy link
Contributor Author

Great question! I haven't looked at the CI setup for this project yet. I have seen commit a9ec3c, which I'm guessing adds configuration to ensure chrono builds on wasm32-unknown-emscripten?

I just checked, and building for wasm32-unknown-emscripten builds without error on HEAD, but wasm32-unknown-unknown does not. Perhaps I could duplicate the work in that commit for wasm32-unknown-unknown?

@djc
Copy link
Member

djc commented May 2, 2022

That sounds great!

@AmateurECE
Add test for wasm_unknown
be4be9f 
This configuration builds plainly for the wasm32-unknown-unknown.
In contrast to the existing wasm_emscripten test, this test ensures
chrono builds on the latest ubuntu without wasm-pack.
@AmateurECE
Copy link
Contributor Author

Okay. As I mentioned, I duplicated the test introduced in the previous commit for wasm32-unknown-unknown. What's interesting to me, is that the existing test, "wasm_simple", appears to test the same target, and it looks like it passed in a recent run, but it fails on my machine.

@AmateurECE
Copy link
Contributor Author

Allow me to clarify: It fails with the three errors I was seeing (Can't leak private type...), in addition to some other errors. After applying my patch, the "can't leak private type" errors disappear, but compilation still fails. I've attached the log, in case anyone is curious.
wasm-pack.txt

@djc
Copy link
Member

djc commented May 10, 2022

So there's not much point in making these changes, and the added CI check is not complete (since it doesn't find these other problems)?

AmateurECE added 2 commits May 11, 2022 20:04
@AmateurECE
Revert "Can't leak crate-private type in stub.rs #662"
1cf381b 
This reverts commit 1663f45.

This is the commit that fixes #662, but I'm temporarily reverting
it to be able to test the CI test on the GitHub pull request.
@AmateurECE
Ensure the default toolchain is used for the wasm_unknown CI test
47e6477 
Issue #210 for actions-rs/toolchain:

    actions-rs/toolchain#210

Indicates that the installed toolchain is not configured to be the
default toolchain for GitHub actions. The OP for that issue
experienced quite a bit of trouble due to this, it seems. Add
configuration to ensure that the current stable toolchain is
being used.
@AmateurECE
Copy link
Contributor Author

I'm actually inclined to think there might be something else going on here. The build for wasm32-unknown-unknown is definitely broken--I tested it with multiple fresh installs today (Docker and spare PCs I have) under the following configs:

  1. Windows 10, installed w/ rustup, rustc 1.58.1, cargo 1.58.0, stable-x86_64-pc-windows-msvc
  2. Ubuntu 22.04, installed w/rustup, rustc 1.60.0, cargo 1.60.0, stable-x86_64-unknown-linux-gnu
  3. Ubuntu 22.04 (same as above), rustc 1.40.0
  4. Ubuntu 22.04 (same as above), rustc 1.41.0

All configurations experienced the three rustc errors this corrects. Additionally, that both the wasm_unknown and wasm_emscripten tests fail on my machine with errors unrelated to this PR, but not in the CI configuration--which kinda makes me think there might be a problem with the CI tests. This issue reported in actions-rs/toolchain might be related, so I reverted my first commit and made the change suggested there.

If the CI fails now, we know that the CI test I added covers the problem that this PR is attempting to fix, and we can go from there :). Thank you for your patience, this ended up being a little more complicated than I originally thought!

@djc
Copy link
Member

djc commented May 12, 2022

Thanks so much for digging into this! CI indeed fails for wasm_unknown now, good stuff!

AmateurECE added 2 commits May 12, 2022 06:17
@AmateurECE
Can't leak crate-private type in stub.rs #662
1ec963e 
First attempt to fix this issue for wasm32-unknown-* targets by
changing visibility of functions not to leak crate-private types.
@AmateurECE
Ensure installed toolchain is default for wasm_simple CI test
980a6cd 
This fix is a hunch based on issue #210 of actions-rs/toolchain
@AmateurECE
Copy link
Contributor Author

No problem, I'm very glad to see it worked! I re-applied the fix, and I also made the same change to the CI config for the wasm_simple test, since that was the one that was failing on my machine. If wasm_simple fails, I may need to come up with another commit to fix whatever it uncovers. If it passes, I'll revert my final commit and we can merge the fix that's been proven.

@AmateurECE
Revert "Ensure installed toolchain is default for wasm_simple CI test"
7235f85 
This reverts commit 980a6cd.

Since this commit didn't have the intended effect on the CI test,
I've reverted it.
@AmateurECE
Copy link
Contributor Author

I reverted my last commit related to the CI test, since it didn't appear to do what I thought it would do. I know there's been a lot of back and forth, so here's where we're at:

  • CI test has been added to test for the problem this PR addresses
  • The build error has been fixed
    Is there anything else I should do or complete before we can merge?

@djc
Copy link
Member

djc commented May 12, 2022

I don't think so! If CI passes I'm happy to merge this.

@djc djc merged commit 7097f93 into chronotope:main May 12, 2022
@djc
Copy link
Member

djc commented May 12, 2022

Thanks for seeing this through!

esheppa pushed a commit to esheppa/chrono that referenced this pull request May 13, 2022
@AmateurECE @esheppa
Fix "can't leak crate-private type in stub.rs" chronotope#662 (chrono…
d0a2916 
…tope#684)

First attempt to fix this issue for wasm32-unknown-* targets by
changing visibility of functions not to leak crate-private types.

Ensure the default toolchain is used for the wasm_unknown CI test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AmateurECE @djc