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

Handle gcs cloud storage file extensions with versioning. #5156

Merged
merged 2 commits into from Jul 14, 2023

Conversation

feuerste
Copy link
Contributor

@feuerste feuerste commented Jun 22, 2023

If a user uses its own IOSystem/IOStream for handling google cloud storage uris, with this PR the extension will still be correct. Together with #5157 the following will be supported without going over all possible importers and trying to detect the format:

Assimp::Importer importer;
importer.SetIOHandler(new GsIOSystem(...));
const aiScene* scene = importer.ReadFile("gs://bucket/model.glb#1234", ...);
...

feuerste added a commit to feuerste/assimp that referenced this pull request Jun 22, 2023
This way we can benefit from e.g. stripping away versioning information
attached behind the actual extension, see also assimp#5156.
feuerste added a commit to feuerste/assimp that referenced this pull request Jun 26, 2023
This way we can benefit from e.g. stripping away versioning information
attached behind the actual extension, see also assimp#5156.
feuerste added a commit to feuerste/assimp that referenced this pull request Jun 26, 2023
This way we can benefit from e.g. stripping away versioning information
attached behind the actual extension, see also assimp#5156.
@turol
Copy link
Member

turol commented Jun 26, 2023

I'm not sure this is a good idea. # is technically a valid character in file names so this might break something for someone.

@feuerste
Copy link
Contributor Author

feuerste commented Jun 26, 2023

I'm not sure this is a good idea. # is technically a valid character in file names so this might break something for someone.

Thanks @turol I agree that # is a valid file name character and we need to make sure that we don't strip too much. gcs tools (see e.g. gsutil: https://github.com/GoogleCloudPlatform/gsutil/blob/c80f329bc3c4011236c78ce8910988773b2606cb/gslib/storage_url.py#L39) can perfectly handle URLs with file versions behind the extension (marked with #), so it would be really useful for everyone extending IOSystem/IOStream with gcs support to already detect the corresponding files based on their extension (see also #5157 in combination with this PR) and not having to wait for the slow fallback where all possible importers try to open the file and read magic bytes or use other ways to detect the type.

feuerste added a commit to feuerste/assimp that referenced this pull request Jun 27, 2023
This way we can benefit from e.g. stripping away versioning information
attached behind the actual extension, see also assimp#5156.
feuerste added a commit to feuerste/assimp that referenced this pull request Jun 28, 2023
This way we can benefit from e.g. stripping away versioning information
attached behind the actual extension, see also assimp#5156.
feuerste added a commit to feuerste/assimp that referenced this pull request Jun 29, 2023
This way we can benefit from e.g. stripping away versioning information
attached behind the actual extension, see also assimp#5156.
@feuerste
Copy link
Contributor Author

feuerste commented Jul 4, 2023

@kimkulling This PR depends on #5157. Once the other one is merged, I will update this one and let you know.

@feuerste feuerste force-pushed the cloud_storage_version branch 3 times, most recently from 5d394a1 to de3913a Compare July 5, 2023 08:01
@turol
Copy link
Member

turol commented Jul 5, 2023

I haven't used C++ regex but have heard many people (even on the committee) call it bad. We probably don't want to use it.

@feuerste feuerste force-pushed the cloud_storage_version branch 2 times, most recently from f000bbc to 51f582c Compare July 5, 2023 09:47
@feuerste
Copy link
Contributor Author

feuerste commented Jul 5, 2023

I haven't used C++ regex but have heard many people (even on the committee) call it bad. We probably don't want to use it.

Fine with me, too.

@feuerste feuerste marked this pull request as draft July 5, 2023 19:58
@feuerste feuerste force-pushed the cloud_storage_version branch 2 times, most recently from 00d2fda to 129bf74 Compare July 11, 2023 08:59
@feuerste feuerste marked this pull request as ready for review July 11, 2023 09:04
@feuerste
Copy link
Contributor Author

@kimkulling This one is ready for review now as well :)

@kimkulling
Copy link
Member

Just working on it.

@kimkulling
Copy link
Member

If I get it right hashes will only get filtered out when they re part of the extension. So for me this looks fine. Some unittests would be great to have.

@feuerste feuerste force-pushed the cloud_storage_version branch 2 times, most recently from 5c626b4 to 36f8d52 Compare July 13, 2023 09:11
@feuerste
Copy link
Contributor Author

If I get it right hashes will only get filtered out when they re part of the extension. So for me this looks fine. Some unittests would be great to have.

Yes, this is correct. I added a few unit tests as well. PTAL!

@feuerste feuerste force-pushed the cloud_storage_version branch 5 times, most recently from 584eb68 to 2bc6416 Compare July 14, 2023 06:18
@feuerste feuerste changed the title Handle cloud storage file extensions with versioning. Handle gcs cloud storage file extensions with versioning. Jul 14, 2023
Copy link
Member

@kimkulling kimkulling left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@kimkulling kimkulling merged commit 5b7ff29 into assimp:master Jul 14, 2023
11 checks passed
@kimkulling
Copy link
Member

Merged, thanks a lot for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants