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

WIP: Change gettext stubs to fix build without gettext. #50

Closed
wants to merge 2 commits into from
Closed

WIP: Change gettext stubs to fix build without gettext. #50

wants to merge 2 commits into from

Conversation

madpilot78
Copy link
Contributor

These changes are required to build audacious on FreeBSD when passing --disable-nls to configure.

  • Change _() to strip const from the string.

This one is used in

list = g_list_sort (list, (GCompareFunc) strcmp);

It fails due to the const qualifier

  • Add a gettext() stub

This is used in

gettext (category.name), -1);

Which does not look FreeBSD specific.

These changes are required to build audacious on FreeBSD when passing --disable-nls to configure.

- Change _() to strip const from the string.

This one is used in https://github.com/audacious-media-player/audacious/blob/73a881fb59bb27f60654b63a95d8c3de73a026ae/src/libaudgui/infowin.cc#L265

It fails due to the const qualifier

- Add a gettext() stub

This is used in https://github.com/audacious-media-player/audacious/blob/73a881fb59bb27f60654b63a95d8c3de73a026ae/src/libaudgui/prefs-window.cc#L534

Which does not look FreeBSD specific.
@madpilot78
Copy link
Contributor Author

Thanks in advance for any feedback.

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Jul 10, 2022
- Updating main port and plugins together
- Add patch to make build succeed when NLS option is off [1]

[1] submitted upstream: audacious-media-player/audacious#50
@jlindgren90
Copy link
Member

Casting away const does not seem right. The code in genre_fill() should be updated instead, preferably to use a type-safe C++ construct such as Index<const char*> rather than C-style GList.

@jlindgren90
Copy link
Member

I believe the call to gettext() in prefs-window.cc should just be replaced with _().

@madpilot78 madpilot78 changed the title Change gettext stubs to fix build without gettext. WIP: Change gettext stubs to fix build without gettext. Jul 27, 2022
@madpilot78
Copy link
Contributor Author

Casting away const does not seem right. The code in genre_fill() should be updated instead, preferably to use a type-safe C++ construct such as Index<const char*> rather than C-style GList.

I'm not very fluent in C++, so I need to study a little to do what you suggest.

For this reason I'm marking this as WIP in the while.

@madpilot78
Copy link
Contributor Author

The code in genre_fill() should be updated instead, preferably to use a type-safe C++ construct such as Index<const char*> rather than C-style GList.

Thinking about this, first of all I can't find any reference regarding Index<>, so I'm not understanding your suggestion.

But I was thinking that using a simple #define _(String) (String) as a noop replacement for gettext does not look correct to me.

Gettext accepts a const char * and then returns a new non const char *. While that mock implementation simply returns back the const char * from the argument, which is a different interface.

Correct thing to do would be to perform an strcpy like operation (preferably using a safe replacement like strlcpy) to a newly allocated char *. This would imitate what gettext is doing.

Not sure where and when such memory should be freed though.

@madpilot78
Copy link
Contributor Author

I also discovered this from getext man page:

BUGS
       The return type ought to be const char *, but is char * to avoid
       warnings in C code predating ANSI C.

Which would mean that simply stripping the const is not really that much different from what gettext is actually doing.

@jlindgren90
Copy link
Member

Thinking about this, first of all I can't find any reference regarding Index<>, so I'm not understanding your suggestion.

Index is a class similar to std::vector, see libaudcore/index.h.

Correct thing to do would be to perform an strcpy like operation (preferably using a safe replacement like strlcpy) to a newly allocated char *. This would imitate what gettext is doing.

As mentioned in the BUGS section you found, gettext() ought to return const char *. If we are modifying the returned string anywhere in our code, that is a bug. So calling strdup() or such is unnecessary. If anything, I would make our _() macro produce a const char * in all cases for consistency.

I've implemented a fix in e6e9016. Thanks for bringing the issue to our attention!

@madpilot78 madpilot78 deleted the Fix_gtk_build_without_gettext branch July 30, 2022 17:41
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.

None yet

2 participants