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

Move self-hosted dependencies to external folder #909

Merged
merged 25 commits into from
May 28, 2024

Conversation

MathiasMagnus
Copy link
Contributor

This PR splits the work prototyped in #889 to have only the file reorg in preparation for Clang-tidy and Clang-format.

Notes for the reviewers

  • The changes here are broken into multiple commits with descriptive names for easier review. I would recommend skipping over changes introduced by commits "Move XYZ to external folder", as they are uninteresting shuffling of unchanged content.
  • The changes are expected to get squashed, as not every commit builds. (Moving and build script updates are in separate commits.)
  • The final commit fixes a hard build error on Windows (when building glloadtests)

@CLAassistant
Copy link

CLAassistant commented May 14, 2024

CLA assistant check
All committers have signed the CLA.

@aqnuep
Copy link
Collaborator

aqnuep commented May 14, 2024

@MarkCallow, this is the first step we already discussed in PR #889, moving the external dependencies to a dedicated external repo. Please review it so that we can merge it ASAP.

@MarkCallow
Copy link
Collaborator

Not sure why you need that fix for gl3loadtests. It's been building on Windows for a very long time.

You still need to fix .reuse/dep5 for some of the moved files. The reuse test on TravisCI is one of the failing jobs.

You need to fix paths in the Web-related sources, interface/js_binding. That is the cause of the other TravisCI failures.

I'll start a proper review later this week. I'm still working on PR #874 (coding not reviewing).

@MarkCallow
Copy link
Collaborator

reuse is still failing related to external/fmt.

I've reviewed the basisu-related changes. They look good. More tomorrow.

@MathiasMagnus
Copy link
Contributor Author

@MarkCallow I fixed the Reuse issues. CI is now failing on the Emscripten bindings, but I believe it is unrelated to the changes here and is a result of the image having changed in the last few hours. The compilation error is not include or link related but a DTOR being explicitly marked deleted.

@MarkCallow
Copy link
Collaborator

@MarkCallow I fixed the Reuse issues. CI is now failing on the Emscripten bindings, but I believe it is unrelated to the changes here and is a result of the image having changed in the last few hours. The compilation error is not include or link related but a DTOR being explicitly marked deleted.

Aargh! Why do people keep making breaking changes?

I am just finishing up major changes to the JS wrapper. I've been using Emscripten 3.1.59. This failing build is using 3.1.60. It is appropriate that the subject copy constructor is marked deleted. I'll probably have to consult the Emscripten people to figure out a fix. I might have to get the major wrapper changes merged to main before you can get the fix. I'll look into it tomorrow. It is late evening here now.

@MarkCallow
Copy link
Collaborator

The changes to Emscripten are apparently intentional. However the fix they suggested makes zero difference. Finding a fix will take more time.

Copy link
Collaborator

@MarkCallow MarkCallow left a comment

Choose a reason for hiding this comment

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

The PR looks good. Hopefully I can find a fix for the Emscripten build quickly.

@MarkCallow
Copy link
Collaborator

Here is a patch for the Emscripten 3.1.60 issue. Please add it to this PR.

ktx_wrapper.patch

@MathiasMagnus
Copy link
Contributor Author

@MarkCallow @aqnuep All lights are green.

@MarkCallow
Copy link
Collaborator

@MathiasMagnus thank you for this well organized piece of work.

@MarkCallow MarkCallow merged commit 5fc739c into KhronosGroup:main May 28, 2024
17 checks passed
@MathiasMagnus MathiasMagnus deleted the external-move branch May 28, 2024 10:07
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.

4 participants