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

Extend glam support up to version 0.24.0. #1038

Merged
merged 5 commits into from
May 30, 2023
Merged

Extend glam support up to version 0.24.0. #1038

merged 5 commits into from
May 30, 2023

Conversation

jnises
Copy link
Member

@jnises jnises commented Apr 19, 2023

We would like update glam version to match other dependencies.

@jnises jnises requested review from eddyb and oisyn as code owners April 19, 2023 14:38
@jnises jnises marked this pull request as draft April 19, 2023 14:46
@eddyb
Copy link
Contributor

eddyb commented Apr 19, 2023

Seems the new copysign stuff causes some issues.

This looks like it might be an issue upstream in glam where compatibility with no_std+libm wasn't checked?
Though, it also feels like it may be limited to target_arch = "spirv" support?

@eddyb
Copy link
Contributor

eddyb commented Apr 20, 2023

I suspect this is blocked on this PR getting into a release (whether 0.23.1 or 0.24.0):

cc @bitshifter (no_std + libm broke in 0.23.0 AFAICT)

@jnises
Copy link
Member Author

jnises commented Apr 20, 2023

If I patch glam to latest main the problem goes away.
So I guess bitshifter/glam-rs#389 solves the issue.

@jnises jnises changed the title Update glam to version 0.23.0 Update glam to version 0.24.0 May 4, 2023
@eddyb
Copy link
Contributor

eddyb commented May 26, 2023

Merge remote-tracking branch 'origin/main' into glam-update-23

Make sure to run git pull --rebase instead of git pull, as these merge commits could cause issues.

Also, remember to mark the commit as "ready for review" and ping me to look at once, once it's 100% ready.

@repi repi removed the request for review from oisyn May 26, 2023 17:06
@jnises
Copy link
Member Author

jnises commented May 26, 2023

Merge remote-tracking branch 'origin/main' into glam-update-23

Make sure to run git pull --rebase instead of git pull, as these merge commits could cause issues.

Also, remember to mark the commit as "ready for review" and ping me to look at once, once it's 100% ready.

Sorry. I had forgotten about this PR 😓
I can have a look at it next week.

@jnises jnises marked this pull request as ready for review May 29, 2023 13:50
@jnises
Copy link
Member Author

jnises commented May 29, 2023

This seems to work for us now.
Would it be better to accept a range of glam versions?
@eddyb

@@ -12,7 +12,7 @@ spirv-std-types.workspace = true
spirv-std-macros.workspace = true
bitflags = "1.2.1"
num-traits = { version = "0.2.14", default-features = false, features = ["libm"] }
Copy link
Contributor

@eddyb eddyb May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
num-traits = { version = "0.2.14", default-features = false, features = ["libm"] }
num-traits = { version = "0.2.15", default-features = false, features = ["libm"] }

... this is all it takes to make 0.23 work, too, I'm so sorry for not noticing this earlier!

(EDIT: unresolved this to leave it visible as an explanation)

Co-authored-by: Eduard-Mihai Burtescu <eddyb@lyken.rs>
Copy link
Contributor

@eddyb eddyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do the range thing now that we know raising the num-traits dependency is enough to support 0.23, and don't forget to add a CHANGELOG entry along the lines of "relaxing glam version requirements" or something (i.e. we should specify the range we support, not just say we updated).

crates/spirv-std/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@eddyb eddyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry for the back & forth, I'll apply these suggestions myself to land this)

crates/spirv-std/Cargo.toml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@eddyb eddyb enabled auto-merge (rebase) May 30, 2023 07:28
@eddyb
Copy link
Contributor

eddyb commented May 30, 2023

Btw, one of the reasons we can keep using a range is that we actually do have a bit of testing, from:

That is, cargo -Z minimal-versions will always pick the lowest endpoint of the range (0.22 even after this PR), so that test will be able to catch anything as obvious as spirv-std using a newer glam type/method.

@eddyb eddyb changed the title Update glam to version 0.24.0 Extend glam support up to version 0.24.0. May 30, 2023
@eddyb eddyb merged commit a686676 into main May 30, 2023
@eddyb eddyb deleted the glam-update-23 branch May 30, 2023 07:38
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