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 compiling for wasm32-unknown-emscripten target #519 #568

Merged
merged 3 commits into from Oct 30, 2021
Merged

Fix compiling for wasm32-unknown-emscripten target #519 #568

merged 3 commits into from Oct 30, 2021

Conversation

orion78fr
Copy link
Contributor

I encountered a bug while compiling for the wasm32-unknown-emscripten target (https://github.com/orion78fr/godot_keepass_rust_totp/runs/2742402148?check_suite_focus=true) and it seems I wasn't the only one : #519

I don't know if you really need (and how to do) a test for this as it seems that it haven't been caught by your CI.

 error[E0428]: the name `inner` is defined multiple times
  --> /github/home/.cargo/registry/src/github.com-1ecc6299db9ec823/chrono-0.4.19/src/sys.rs:25:1
   |
21 | mod inner;
   | ---------- previous definition of the module `inner` here
...
25 | mod inner;
   | ^^^^^^^^^^ `inner` redefined here
   |
   = note: `inner` must be defined only once in the type namespace of this module
@jcaesar
Copy link

jcaesar commented Jul 17, 2021

If wasm32-unknown-emscripten is cfg(unix), wouldn't it be better to use the unix mod for it, instead of the stub one? It does compile, but I haven't tested whether it does anything meaningful.

(Nevermind the fact that I submitted a PR to fix the same problem without looking whether somebody already did. Sorry for that.)

@Milo123459 Milo123459 merged commit f6bd567 into chronotope:main Oct 30, 2021
@hoodmane
Copy link

hoodmane commented Apr 8, 2022

wouldn't it be better to use the unix mod for it, instead of the stub one? It does compile, but I haven't tested whether it does anything meaningful.

Yes it is better. Emscripten has working implementations of timegm, mktime, and localtime_r.

@hoodmane
Copy link

hoodmane commented Apr 9, 2022

Both #574 and #607 make the correct choice to use the unix one on emscripten target but this PR doesn't. Hopefully one of those two PRs can be merged.

@orion78fr
Copy link
Contributor Author

@hoodmane #593 has been merged in the meantime, no need to comment on an old merged PR from 6 month ago

@hoodmane
Copy link

hoodmane commented Apr 9, 2022

Ah great, thanks!

@djc
Copy link
Contributor

djc commented Apr 10, 2022

Actually the fix was merged after @hoodmane made their comment and their comment helped me find #593. So thanks for that!

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.

None yet

5 participants