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

Add event config for starboard #4228

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MSoliankoLuxoft
Copy link
Collaborator

@MSoliankoLuxoft MSoliankoLuxoft commented Oct 8, 2024

b/372056053

@niranjanyardi
Copy link
Contributor

With this change in, can you build PS5 locally ? @MSoliankoLuxoft

@kaidokert , @andrewsavage1 :
I'm a bit worried that this will affect other platforms unintentionally.
Also this seems to be a newly needed change just for PS5 as we updated the SDK to SDK10 .
In the pre-chrobalt repo it's not there: https://paste.googleplex.com/6574054365724672

Maybe a good way to handle this is to introduce a macro USE_STARBOARD_3P - which is set by 3P owners instead of our USE_STARBOARD or maybe there's a better way to handle this . wdyt ?

@andrewsavage1
Copy link
Contributor

@kaidokert , @andrewsavage1 : I'm a bit worried that this will affect other platforms unintentionally. Also this seems to be a newly needed change just for PS5 as we updated the SDK to SDK10 . In the pre-chrobalt repo it's not there: paste.googleplex.com/6574054365724672

Maybe a good way to handle this is to introduce a macro USE_STARBOARD_3P - which is set by 3P owners instead of our USE_STARBOARD or maybe there's a better way to handle this . wdyt ?

I don't think we should be accepting this kind of change at al unless it ends up being necessary for evergreen builds. Pretty much all customizations for 3pp should be maintained downstream

@niranjanyardi
Copy link
Contributor

@MSoliankoLuxoft can you somehow fix this via setting buildflags/ defines in GN in the PS5 gn files ? not these common files.

Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

My recommendation:

In PS5 port, create a directory

internal/third_party/libevent - put this modified file there. add internal/ to root include paths.

Then make sure that this added internal include path comes before the other include paths - so that compiler always finds it before this file.

Updated `#include` for `event-config.h` to support platform-specific configurations.
Now, if `LIBEVENT_CONFIG_PLATFORM_HEADER` is defined, it includes the specified
platform config file; otherwise, it defaults to `event-config.h`.

b/372056053

Signed-off-by: Mykola Solianko <[email protected]>
@MSoliankoLuxoft
Copy link
Collaborator Author

My recommendation:

In PS5 port, create a directory

internal/third_party/libevent - put this modified file there. add internal/ to root include paths.

Then make sure that this added internal include path comes before the other include paths - so that compiler always finds it before this file.

Unfortunately, the proposed approach does not work because the event-config.h file, which I want to override in the PS5 build, is included as "event-config.h". When I change the path to my custom directory in the PS5 GN file, it doesn’t work because it will always take the local version of event-config.h located in third_party/libevent. For it to work, the inclusion would need to change from "" to <event-config.h>, but unfortunately, such changes would break most builds.

I also tried using the following define in the PS5 GN file: defines=["event-config.h=third_party/libevent/starboard/event-config.h"], but it didn’t work because there is a - character in event-config.h.

I decided to use an alternative approach, similar to what is used in epoll.c with the LIBEVENT_PLATFORM_HEADER define, which for PS5 is defined in internal\starboard\shared\playstation\configuration_public.h and specifies the necessary path for the correct include.

Please let me know if this approach would be acceptable.

@@ -38,7 +38,12 @@
extern "C" {
#endif

#if defined LIBEVENT_CONFIG_PLATFORM_HEADER
Copy link
Member

@kaidokert kaidokert Oct 30, 2024

Choose a reason for hiding this comment

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

This is a better approach, but maybe this block fits better inside of the "event-config.h" at the top before the other conditions ?

That change could even be sent to upstream and be accepted, i think ?

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, it's not really an upstreaming concern for us much longer, Chromium removed libevent entirely: https://issues.chromium.org/issues/40057013

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.

5 participants