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

Enhance game export, re-add embedded PCK option #24086

Merged
merged 2 commits into from Jul 5, 2019

Conversation

@RandomShaper
Copy link
Member

RandomShaper commented Nov 30, 2018

UPDATE: Marked as WIP since the appended PCK approach has issues and a better way has to be implemented.
UPDATE 2: The better way is implemented now. It may still have some issues but I has worked flawlessly in my tests.

Two commits:

Add embedded PCK option to PC platforms

The basic point is as in 2.1 (appending the PCK into the executable), but this implementation also patches a dedicated section in the ELF/PE executable so that it matches the appended data perfectly.

This way, the runtime code can find the PCK very easily by just looking at the end of the file, not having to parse the executable headers to find the special section. That's only needed during export.

The usage of integer types is simplified in existing code; namely, using plain int for small quantities.

Skip export of non-project libraries

That is, any library referred to in GDNative library resources, won't be copied to the export target path unless its path begins with res://.

The case use for this is a bit advanced: having a GDN library that will be deployed separately from the project; for instance, to a path in the system (like /opt/...).

Currently the GDN library editor doesn't allow to pick dynamic libraries outside the project, but that can be done by editing the .gdnlib file manually.


This code is donated by AdPodnet.


Note for @hpvb: no cherry-picking needed. Dedicated PR for 3.0 is #24087. UPDATE: 3.0-specific PR won't be worked on.

@RandomShaper RandomShaper added this to the 3.0 milestone Nov 30, 2018
@RandomShaper RandomShaper requested review from karroffel and reduz as code owners Nov 30, 2018
@RandomShaper RandomShaper modified the milestones: 3.0, 3.1 Nov 30, 2018
@RandomShaper RandomShaper changed the title Enhance game export Enhance game export [wip] Nov 30, 2018
@karroffel

This comment has been minimized.

Copy link
Contributor

karroffel commented Dec 3, 2018

GDNative parts seem reasonable 👍

@RandomShaper RandomShaper force-pushed the RandomShaper:bundle-pck-to-executable branch 2 times, most recently from 8a69ff5 to f1be962 Dec 3, 2018
@RandomShaper RandomShaper changed the title Enhance game export [wip] Enhance game export Dec 3, 2018
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Dec 4, 2018

@RandomShaper

This comment has been minimized.

Copy link
Member Author

RandomShaper commented Dec 6, 2018

I have yet to fix issues with cross-compilation and add CI checks for the special section. I'll do that ASAP.

@RandomShaper RandomShaper force-pushed the RandomShaper:bundle-pck-to-executable branch from f1be962 to 6539f56 Dec 7, 2018
@RandomShaper RandomShaper changed the title Enhance game export Enhance game export [WIP] Dec 8, 2018
@RandomShaper RandomShaper force-pushed the RandomShaper:bundle-pck-to-executable branch from 6539f56 to 643b69a Dec 11, 2018
@RandomShaper

This comment has been minimized.

Copy link
Member Author

RandomShaper commented Dec 11, 2018

@akien-mga, I've updated the PR supporting MinGW as well.

Now, regarding what I told you about CI checks, could you lend me a hand?

What needs to be checked is this:

  • for MSVC compilation, the command dumpbin <template_executable> /SECTION:pck prints at least something like this:
     pck name
       8 virtual size
  • for MinGW compilation (Windows) and GCC/Clang compilation (X11), the output of the command objdump -h <template_executable> includes something like this:
  3 pck           00000008  0000000001db7000  0000000001db7000  019b4600  2**3
                  CONTENTS, ALLOC, LOAD, READONLY, DATA

For all the cases, it really only matters that the pck section appears (first line in both cases). Other data can change and by having its presence checked we get enough peace of mind.

If you can help, feel free to commit to this branch.

@RandomShaper

This comment has been minimized.

Copy link
Member Author

RandomShaper commented Dec 11, 2018

I don't really think you can commit to this branch. XD

Well, you can always add a commit later if this is merged. We can consider the CI checks something extra.

@RandomShaper RandomShaper changed the title Enhance game export [WIP] Enhance game export Dec 11, 2018
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Dec 12, 2018

Moving to 3.2 milestone as we're about to enter the beta stage and release freeze for Godot 3.1. Only bug fixes against issues of the 3.1 milestone (or enhancements to core features of the 3.1 roadmap) will be considered until 3.1-stable is released.

Note: If it's merged after 3.1-stable is released, this change may be considered for cherry-picking into a later 3.1.x release.

@akien-mga akien-mga modified the milestones: 3.1, 3.2 Dec 12, 2018
@RandomShaper RandomShaper force-pushed the RandomShaper:bundle-pck-to-executable branch from 643b69a to 6b45424 Mar 12, 2019
@RandomShaper RandomShaper force-pushed the RandomShaper:bundle-pck-to-executable branch 2 times, most recently from cb469f8 to 4583d8e Mar 20, 2019
@RandomShaper RandomShaper force-pushed the RandomShaper:bundle-pck-to-executable branch from 4583d8e to 8419119 Mar 25, 2019
That is, any library referred to in GDNative library resources, won't be copied to the export target path unless its path begins with `res://`.

The case use for this is a bit advanced: having a GDN library that will be deployed separately from the project; for instance, to a path in the system (like `/opt/...`).

Currently the GDN library editor doesn't allow to pick dynamic libraries outside the project, but that can be done by editing the `.gdnlib` file manually.
The basic point is as in 2.1 (appending the PCK into the executable), but this implementation also patches a dedicated section in the ELF/PE executable so that it matches the appended data perfectly.

The usage of integer types is simplified in existing code; namely, using plain `int` for small quantities.
@RandomShaper RandomShaper force-pushed the RandomShaper:bundle-pck-to-executable branch from 8419119 to 40f4d3c Jul 3, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 4, 2019

Sorry for the late review. The first commit is a pretty simple and could be merged right away.

For the second commit, code changes seem fine, but I'd like it discussed with @reduz (to ensure he accepts to bring back the "embed PCK" feature) and @hpvb (regarding the extra linker stuff on X11).

@akien-mga akien-mga requested review from hpvb and removed request for karroffel Jul 4, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 4, 2019

@reduz said "if it works properly per platform, I think it's fine, the main problem is that the way I did it back then was way too hacked, and sometimes the binary would be detected as invalid".

I see you added a PCK section for Windows and X11, but what about macOS? Doesn't it need special handling too?

@RandomShaper

This comment has been minimized.

Copy link
Member Author

RandomShaper commented Jul 4, 2019

The option is not available for Mac.
It's for the ExportPlatformPC, which includes Windows and X11.
For Mac, its export platform class is aside and I didn't see a clear path for it.
I may revisit it in a future, but this is how far I was able to get.
Aside, the CI checks would be interesting to add, but I need help for those.

@RandomShaper

This comment has been minimized.

Copy link
Member Author

RandomShaper commented Jul 4, 2019

In my tests it worked properly and there were no false positives from AVs I tried.
Nonetheless, it would be good to have this for alphas and betas so people can try it.
I'll try to fix any issues that arise.

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 5, 2019

Thanks for the clarification, let's give it a spin then :)

@akien-mga akien-mga merged commit a149e41 into godotengine:master Jul 5, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 5, 2019

Thanks!

@RandomShaper RandomShaper deleted the RandomShaper:bundle-pck-to-executable branch Jul 5, 2019
@akien-mga akien-mga changed the title Enhance game export Enhance game export, re-add embedded PCK option Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.