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

Fix to restore library_path as absolute path #94373

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

maiself
Copy link
Contributor

@maiself maiself commented Jul 15, 2024

Fix for possible issue with library_path being set as a local resource path rather than an absolute file system path. May have broken the Python bindings, and possibly any other extension needing an absolute path. Needs a little more investigation, marking as draft for now. @dsnopek, could you look this over too?

Related:

@dsnopek
Copy link
Contributor

dsnopek commented Jul 15, 2024

Ack! Thanks for catching this. As you point out (via linking PR #84620), this isn't the first time we've accidentally broken this. I think we should probably add a test case to the godot-cpp tests to make sure the path is an absolute path, so we don't break it again.

The change in your PR looks fine to me, although, I haven't done any testing.

@dsnopek dsnopek modified the milestones: 4.4, 4.3 Jul 15, 2024
@maiself
Copy link
Contributor Author

maiself commented Jul 15, 2024

I think we should probably add a test case to the godot-cpp tests to make sure the path is an absolute path, so we don't break it again.

I was thinking last night about adding something like a ERR_FAIL_COND_V_MSG to this function to catch this kind of mistake earlier if it should happen again. And possibly rename some variables for clarity, but that's getting more involved.

I'm not sure what a godot-cpp test case for this world look like, if you think that's the best way to address this can you put that together?

@dsnopek
Copy link
Contributor

dsnopek commented Jul 15, 2024

I just posted PR godotengine/godot-cpp#1520 which adds a godot-cpp test.

I was thinking last night about adding something like a ERR_FAIL_COND_V_MSG to this function to catch this kind of mistake earlier if it should happen again.

I'm not sure that makes sense in this case. Those error macros are generally used for errors in input to a function, whereas this is an error of output. We also won't necessarily see that error during development (ie it wouldn't fail CI on PRs that break this), but if godot-cpp starts failing tests, then we know there's an issue to look into.

@dsnopek
Copy link
Contributor

dsnopek commented Jul 15, 2024

Oh, and in working on that test, I did test that it fails without this PR and succeeds with it, so I have now tested this PR as well!

Can you take it out of draft?

@maiself
Copy link
Contributor Author

maiself commented Jul 15, 2024

All sounds good to me. Good point about the error messages and input vs output.

@maiself maiself marked this pull request as ready for review July 15, 2024 17:14
@maiself maiself requested a review from a team as a code owner July 15, 2024 17:14
@akien-mga akien-mga merged commit 31f696c into godotengine:master Jul 17, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

4 participants