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

Update base64 requirement from 0.13.0 to 0.21.5 #10336

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Update base64 requirement from 0.13.0 to 0.21.5 #10336

merged 2 commits into from
Dec 21, 2023

Conversation

mnmaita
Copy link
Member

@mnmaita mnmaita commented Nov 1, 2023

Objective

Solution

  • Bumped base64 requirement and manually migrated code to fix a breaking change after updating.

@mockersf mockersf added the C-Dependencies A change to the crates that Bevy depends on label Nov 1, 2023
@Friz64
Copy link
Contributor

Friz64 commented Nov 3, 2023 •
edited
Loading

This new API is ridiculous. See this thread: https://nitter.net/mycoliza/status/1708933310548935148

I suggest we switch to data-encoding.

james7132
james7132 approved these changes Nov 26, 2023
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Both data-encoding and base64 are now in our dependency tree. It'd be ideal to deduplicate, but ron depends on [email protected] right now. It's probably best to merge this now and look to get deduplicate across ron and bevy_gltf in a later PR.

@mnmaita mnmaita disabled auto-merge December 2, 2023 01:36
@mnmaita
Copy link
Member Author

mnmaita commented Dec 2, 2023

@james7132 I noticed this got stuck in the merge queue due to a never-starting job so I just updated the PR.

I also noticed that some bevy crates target ron ^0.8 and some others 0.8.0. 0.8.0 depends on base64 ^0.13 while 0.8.1 on ^0.21. Should we try to change the requirement for ron to be 0.8 across the board to consolidate? I can do this as a follow up.

@mnmaita
Copy link
Member Author

mnmaita commented Dec 20, 2023

@mockersf @alice-i-cecile @james7132 bumping this one as it was added to the merge queue a while ago but since one of the CI jobs hanged it never got picked up. Could any of you please check it again and merge if you think it's good to go?

@mockersf mockersf added this pull request to the merge queue Dec 21, 2023
@mockersf
Copy link
Member

I also noticed that some bevy crates target ron ^0.8 and some others 0.8.0. 0.8.0 depends on base64 ^0.13 while 0.8.1 on ^0.21. Should we try to change the requirement for ron to be 0.8 across the board to consolidate? I can do this as a follow up.

It's already taking the latest version, you can check with cargo tree --invert ron --target all

Merged via the queue into bevyengine:main with commit 3b59dbd Dec 21, 2023
26 checks passed
@mnmaita mnmaita deleted the up-base64 branch December 21, 2023 01:41
@mockersf mockersf mentioned this pull request Dec 21, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 21, 2023
@mockersf
fix base64 padding when loading a gltf file (#11053)
8666b07 
# Objective

- After #10336, some gltf files fail to load (examples
custom_gltf_vertex_attribute, gltf_skinned_mesh, ...)
- Fix them

## Solution

- Allow padding in base 64 decoder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james7132 james7132 james7132 approved these changes

@mockersf mockersf mockersf approved these changes

Assignees
No one assigned
Labels
C-Dependencies A change to the crates that Bevy depends on
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mnmaita @Friz64 @mockersf @james7132