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

Remove unused includes to improve portability #2

Merged
merged 1 commit into from
Oct 20, 2022

Conversation

hannobraun
Copy link
Contributor

The presence of those includes limits the platform support of this library. Specifically, cross-compiling to wasm32-unknown-unknown is not possible.

These are only used in commented code, it seems, so this compiles without further changes. I've confirmed this by compiling against the Rust Tier 1 targets1 except for the following:

  • i686-pc-windows-msvc: I'm on Linux and don't know how to cross- compile for that.
  • i686-unknown-linux-gnu: The required GCC version is not packaged for my distribution, and I failed to install it via other means.

Some of those builds I've done locally, other indirectly, by building a version of Fornjot that includes this commit on GitHub Actions:

In addition, I compiled locally for the following targets, all of which worked without problems:

  • wasm32-unknown-unknown
  • wasm32-wasi

Footnotes

  1. https://doc.rust-lang.org/rustc/platform-support.html#tier-1-with-host-tools

The presence of those `include`s limits the platform support of this
library. Specifically, cross-compiling to `wasm32-unknown-unknown` is
not possible.

These are only used in commented code, it seems, so this compiles
without further changes. I've confirmed this by compiling against the
Rust Tier 1 targets[^1] except for the following:

- `i686-pc-windows-msvc`: I'm on Linux and don't know how to cross-
  compile for that.
- `i686-unknown-linux-gnu`: The required GCC version is not packaged for
  my distribution, and I failed to install it via other means.

Some of those builds I've done locally, other indirectly, by building a
version of Fornjot that includes this commit on GitHub Actions:

- https://github.com/hannobraun/Fornjot/actions/runs/3249616834/jobs/5332219174
- https://github.com/hannobraun/Fornjot/actions/runs/3249616834/jobs/5332219243
- https://github.com/hannobraun/Fornjot/actions/runs/3249616834/jobs/5332219325

In addition, I compiled locally for the following targets, all of which
worked without problems:

- wasm32-unknown-unknown
- wasm32-wasi

[^1]: https://doc.rust-lang.org/rustc/platform-support.html#tier-1-with-host-tools
hannobraun added a commit to hannobraun/fornjot that referenced this pull request Oct 14, 2022
The upstream version can't be compiled to `wasm32-unknown-unknown`.

My fork includes the following pull requests:

- hporro/robust-predicates#1
- hporro/robust-predicates#2
@hporro
Copy link
Owner

hporro commented Oct 18, 2022

I should be able to test in the platforms you haven't. I'll do it in the following days!

@hannobraun
Copy link
Contributor Author

Thank you, sounds great!

@hporro
Copy link
Owner

hporro commented Oct 20, 2022

Actually, I misunderstood and it's quite difficult to test those configurations for me too (and I don't quite have a lot of time). Still, I'll merge this.

@hporro hporro merged commit 2f80a51 into hporro:master Oct 20, 2022
@hporro
Copy link
Owner

hporro commented Oct 20, 2022

I also published a new version of the crate, if you want to use it with these changes :)

@hannobraun
Copy link
Contributor Author

Actually, I misunderstood and it's quite difficult to test those configurations for me too (and I don't quite have a lot of time). Still, I'll merge this.

Thank you, @hporro! For what it's worth, I think it's quite unlikely that there are any build problems with the configuration we weren't able to test.

I also published a new version of the crate, if you want to use it with these changes :)

Nice! I'll upgrade today 😄

@hannobraun hannobraun deleted the wasm branch October 20, 2022 08:43
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.

2 participants