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

speed-dreams: update to 2.2.3 #32474

Merged
merged 1 commit into from
Aug 30, 2021
Merged

speed-dreams: update to 2.2.3 #32474

merged 1 commit into from
Aug 30, 2021

Conversation

r-ricci
Copy link
Contributor

@r-ricci r-ricci commented Aug 12, 2021

General

Have the results of the proposed changes been tested?

  • I use the packages affected by the proposed changes on a regular basis and confirm this PR works for me
  • I generally don't use the affected packages but briefly tested this PR

@r-ricci
Copy link
Contributor Author

r-ricci commented Aug 13, 2021

On x86_64-musl it segfaults in a chroot but works in a VM.

nocross=yes
replaces="speed-dreams-data>=0"
CFLAGS=-fpermissive
CXXFLAGS=-fpermissive
LDFLAGS="-Wl,--no-as-needed"
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining this? I suppose it depends on some library's constructor...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you add a comment explaining this? I suppose it depends on some library's constructor...

Honestly I don't know why --as-needed is not supported, I just know it makes the build fail.
I will report it to upstream. I see they had the same problem in the past and then it was fixed, this is a regression.

Copy link
Member

Choose a reason for hiding this comment

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

Can you mention in the commit message as well, then? Or even add a comment on top, that removal should be attempted when updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When cmake policy CMP0072 is set to NEW, linking with --as-needed fails. Should I patch CMakeLists.txt to use the legacy library or just use --no-as-needed?

Copy link
Member

Choose a reason for hiding this comment

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

Huh, I didn't expect that. Theoretically we should pass it for all Cmake builds, since they are eventually going to start using that policy...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if other packages are affected.
Anyway, another workaround could be to pass -lGL only when linking the problematic files (I tested this and it works), but mixing legacy and new libraries is ugly.

@ericonr
Copy link
Member

ericonr commented Aug 13, 2021

And document in the commit the x86_64-musl crash, for posterity :p

@r-ricci r-ricci changed the title speed-dreams: update to 2.2.3. [WIP] speed-dreams: update to 2.2.3. Aug 13, 2021
Comment on lines 1 to 12
--- a/src/modules/simu/simuv2.1/SOLID-2.0/src/C-api.cpp
+++ b/src/modules/simu/simuv2.1/SOLID-2.0/src/C-api.cpp
@@ -60,9 +60,7 @@
typedef map<DtObjectRef, Object *> ObjectList;
typedef set<Encounter> ProxList;

-#if defined( WIN32) || (__APPLE__)
#define uint unsigned int
-#endif

PointBuf pointBuf;
IndexBuf indexBuf;
Copy link
Member

Choose a reason for hiding this comment

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

this file should probably be named musl.patch, not musl.diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aren't diff and patch synonyms? (I mean the files, not the programs). ".diff" is my personal preference, but I'll rename it if that's preferred to keep consistency.

Copy link
Member

Choose a reason for hiding this comment

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

I think we don't really use diff in the repo, even if it's supported.

@r-ricci r-ricci changed the title [WIP] speed-dreams: update to 2.2.3. speed-dreams: update to 2.2.3 Aug 17, 2021
@ericonr
Copy link
Member

ericonr commented Aug 30, 2021

How far does it get for you in a musl VM? It segfaults here, right after complaining that shadow.xml is missing:

00:00:00.647 Default  Error   No usable 'shadow' driver (shadow.xml not found or not readable)

It even gets to play the startup sound and show the first screen.

@ericonr
Copy link
Member

ericonr commented Aug 30, 2021

Found it, I think. Seems to be due to thread stack size.

"-Wl,--no-as-needed" is required to avoid a linker error which occurs
when linking with `-lOpenGL`.

"-Wl,-z,stack-size=2097152" is required to avoid a stack overflow on
threads, when running on musl. Without it, overflow happens in the
openalmusicplayer::streambuffer() function, which allocates a sound
buffer on the stack.

Co-authored-by: Érico Nogueira <erico.erc@gmail.com>
@ericonr
Copy link
Member

ericonr commented Aug 30, 2021

That was it. Merging now.

@ericonr ericonr merged commit 6faa59a into void-linux:master Aug 30, 2021
@r-ricci r-ricci deleted the speed-dreams branch August 30, 2021 10:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants