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 CI builder #23

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

Add CI builder #23

wants to merge 1 commit into from

Conversation

jpcima
Copy link

@jpcima jpcima commented May 8, 2021

This adds a builder with github actions.
There are builds on 2 distributions: the first one passes, and the second illustrates the linker problems which are corrected by PR #22.

@nomadbyte
Copy link
Owner

I understand that you got the link errors on Arch linux. I wonder what's different there vs Ubuntu/Debian in the way the linker resolving references?

@jpcima
Copy link
Author

jpcima commented May 8, 2021

Having just checked the package comments, it would appear that these problems relate to gcc 10.

@nomadbyte
Copy link
Owner

nomadbyte commented May 8, 2021

I just cleaned up (in 3015be7) the use of brightonEvent event variable that was flagged in the linking on Arch. In both instances it was rather benign, however it was indeed incorrect use, as the event normally should be a local scope variable. Thanks for catching this.

Did you get similar link errors on the note_diff use too?

@jpcima
Copy link
Author

jpcima commented May 8, 2021

Did you get similar link errors on the note_diff use too?

Yes, that one and lots of others, but all this is already fixed in the PR #22
I've made variables static wherever applicable.

@nomadbyte
Copy link
Owner

Turns out -fno-common is now a default in GCC 10.

@jpcima
Copy link
Author

jpcima commented May 8, 2021

so if I get this right, these global variable must act as if they're shared?
Ok I should take a second look #22 to see if I preserve the behavior correctly.

On another note, is this CI stuff ok for merging?

@nomadbyte
Copy link
Owner

nomadbyte commented May 8, 2021

I have not decided yet on the way for packaging (or specific target platforms, if any); this will need some tweaking. So for now I postpone the automation of the builds and releases; after all, there's really no test suite yet to run to validate the pushes.

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.

2 participants