Skip to content

Ensure packaged assemblies are used in c sharp #109935

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ensure packaged assemblies are used in c sharp #109935

wants to merge 1 commit into from
+33 −17

Conversation

the-bham
Copy link

@the-bham the-bham commented Aug 24, 2025 •
edited
Loading

Issue: currently running the command line to launch a pack will define prepoccesor TOOLS_ENABLED forcing godot to look in the tools assemblies only, and misses packaged assemblies. so c# projects will fail instantiate from command line

Fix : will prioritize packaged assemblies before trying to access tools assemblies when a project is run from the command line.

@fire fire changed the title added some properties and logic to ensure packaged assemblies are use… Add properties and logic to ensure packaged assemblies are used in c sharp Aug 24, 2025
This was referenced Aug 24, 2025
Open
Open
@the-bham the-bham marked this pull request as ready for review August 25, 2025 01:04
@the-bham the-bham requested a review from a team as a code owner August 25, 2025 01:04
@the-bham the-bham marked this pull request as draft August 25, 2025 07:42
@AThousandShips AThousandShips added this to the 4.x milestone Aug 25, 2025
@the-bham the-bham marked this pull request as ready for review August 25, 2025 17:19
@the-bham
Copy link
Author

the-bham commented Aug 25, 2025 •
edited
Loading

i havent tested this build on linux (or any other platform than windows) yet since I am having difficulty with buildsystem in fedora42. I dont see why anything here would break other platforms though.

@AThousandShips
Copy link
Member

Realized that this targets the 4.4 branch, this is not correct, it should target the master branch, and if it's considered valid it can be cherry picked for 4.4, but this is a new feature so should go on master

@the-bham
Copy link
Author

the-bham commented Aug 25, 2025 •
edited
Loading

Realized that this targets the 4.4 branch, this is not correct, it should target the master branch, and if it's considered valid it can be cherry picked for 4.4, but this is a new feature so should go on master

I noticed that on initial PR rules and looked at merging to 4.5 but there was significant refactor needed (which I will happily do just need to find the time...) was hoping this would at least help 4.4.x as I've seen plenty of issues reported caused by this where users go down holes and probably eventually give up and move on.

@AThousandShips
Copy link
Member

I'd say that's a good argument for moving it to master, it won't be approved for just 4.4 unless it's only relevant for 4.4

@the-bham
Copy link
Author

should I update PR and change base to master (its warning me of clearing commits and comments), or close PR and open a new one with refactored code to master branch?

@AThousandShips
Copy link
Member

I'd suggest:

  • Convert it to a draft
  • Change the target to master
  • Rebase the branch, see here

Making it a draft will make sure it doesnn't cause a lot of unnecessary review requests

@the-bham the-bham marked this pull request as draft August 25, 2025 17:38
@the-bham the-bham changed the base branch from 4.4 to master August 25, 2025 17:39
@the-bham the-bham force-pushed the prioritize-packaged-assemblies branch from 09102eb to c8598f9 Compare August 25, 2025 18:16
@AThousandShips
Copy link
Member

There! That seems to have gone smoothly!

@the-bham
Copy link
Author

There! That seems to have gone smoothly!

smoothest rebase ive ever experienced but i was still sweating

@the-bham the-bham closed this Aug 25, 2025
@the-bham the-bham force-pushed the prioritize-packaged-assemblies branch from e20d5a9 to 3defc85 Compare August 25, 2025 18:31
@the-bham the-bham reopened this Aug 25, 2025
@the-bham the-bham changed the title Add properties and logic to ensure packaged assemblies are used in c sharp Ensure packaged assemblies are used in c sharp Aug 25, 2025
@the-bham
Copy link
Author

ok wow 1 hour to refactor and another 4 or so to fix my dotnet for 4.5 and glue it all back together. I believe my binary is working correctly now. I will try again in linux fedora 42 as I believe I understand what I was doing wrong in that build process. my only other concern is I added some to the ANDROID_ENABLED block but will need to build for that environment to test.

@the-bham the-bham force-pushed the prioritize-packaged-assemblies branch 3 times, most recently from 1f209be to 6b2f4b5 Compare August 26, 2025 00:56
@the-bham the-bham force-pushed the prioritize-packaged-assemblies branch 2 times, most recently from 920869b to 0b0eae2 Compare August 26, 2025 00:58
@the-bham the-bham marked this pull request as ready for review August 26, 2025 00:59
@the-bham
Copy link
Author

the-bham commented Aug 26, 2025 •
edited
Loading

tested my minimum reproducible project in my custom linux editor and that seems to work just fine. tested my own personal project which lead me to fix this issue in the first place. finally I have my csharp dedicated headless server running in kubernetes agones!

wouldnt be able to do that efficiently if I could not run the godot main pack from the command line

@the-bham
prioritize packaged assemblies in tools_enabled binaries
4fb8a0e 
--tools_assemblies should only be set if tools_enabled

-- setting packaged_assemblies_dir when ANDROID_ENABLED

use tools assemblies if TOOLS_ENABLED
@the-bham the-bham force-pushed the prioritize-packaged-assemblies branch from 0b0eae2 to 4fb8a0e Compare September 19, 2025 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
breaks compat enhancement topic:dotnet
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.
2 participants
@the-bham @AThousandShips