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

Workflow Updates #205

Merged
merged 54 commits into from
Oct 3, 2022
Merged

Workflow Updates #205

merged 54 commits into from
Oct 3, 2022

Conversation

dethrace-labs
Copy link
Owner

@dethrace-labs dethrace-labs commented Oct 2, 2022

  • simplifies workflow yaml
  • stops double-running workflow on pull request and push. Workflow can be forced to run without opening a PR by naming your branch in the format ci-*
  • generates downloadable artifacts on each push to main
  • releases named in the form of Dethrace 0.4.0 rather than Dethrace v0.4.0 to match the more common convention

@@ -9,4 +15,6 @@ cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DBUILD_TESTS=ON -DDETHRACE_WERROR=ON -B
cmake --build build -- -j 4
Copy link
Collaborator

@madebr madebr Oct 2, 2022

Choose a reason for hiding this comment

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

I think you need to add -DCMAKE_OSX_ARCHITECTURES=x86_64 (or amd64 idk) to be sure this does not build an armv8 executable.
Speaking of armv8, perhaps add armv8 to the build matrix for macos?

See CMAKE_OSX_ARCHITECTURES.
I think you can build a "fat" executable by doing -DCMAKE_OSX_ARCHITECTURES="x86_64;armv8", but don't quote me on that :)

Copy link
Owner Author

@dethrace-labs dethrace-labs Oct 2, 2022

Choose a reason for hiding this comment

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

thanks. github actions don't yet support arm osx builds (actions/runner-images#2187), so I've set it just to x86_x64 for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need M1 hardware. We just want to cross build to armv8.

Copy link
Collaborator

@madebr madebr Oct 2, 2022

Choose a reason for hiding this comment

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

Actually, we do when we run the tests. So you're right.
Unless we disable them for that platform alone.

# package artifact
tar -czvf dethrace-${BUILD_TAG}-darwin-amd64.tar.gz build/dethrace

echo "::set-output name=filename::dethrace-${BUILD_TAG}-darwin-amd64.tar.gz"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just cosmetic, but can you please add a final new line to all files to avoid GitHub adding this marker.

Copy link
Owner Author

Choose a reason for hiding this comment

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

thanks. I moved to a new laptop and forgot to turn on the auto-insert newlines setting 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

On my dev machine, I added a .editorconfig in the root folder of my user home dir containing:

root = true

[*]
end_of_line = lf
insert_final_newline = true

[Makefile]
indent_style = tab
indent_size = 4

[*.py]
indent_style = space
tab_width = 4

And all my ide's are configured to use editorconfig files.

When some project has another default, I add an .editorconfig file in that projects root directory.
To avoid git picking up that file, I added .editorconfig to .gitignore in my user home directory.

fi

# install deps
brew install SDL2
Copy link
Collaborator

Choose a reason for hiding this comment

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

On Linux and Macos, we implicitly assume people have SDL2 somehow installed.
Should we add libSDL2.(dylib|so) to the archive?
In that case, we might need to fix rpaths.

Food for thoughts, and for a later pr.

Copy link
Owner Author

Choose a reason for hiding this comment

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

yep - I ran into this problem on a different laptop where libsdl2 was not in the expected place. Definitely an issue to fix - I think as part of generating a fat .app package for macos

MATRIX_PLATFORM: ${{ matrix.platform }}
run: .github/scripts/build-linux.sh
id: build
run: .github/scripts/build-${{ runner.os }}.sh
Copy link
Collaborator

@madebr madebr Oct 2, 2022

Choose a reason for hiding this comment

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

Can we collapse the windows job into this job by adding the build script into the matrix?
e.g.:

matrix:
    include:
      - os: 'ubuntu-latest'
        script: '.github/scripts/build-linux.sh'
      - os: 'macos-latest'
        script: '.github/scripts/build-macos.sh'
      - os: 'windows-latest'
        script: '.github\\scripts\\build-msvc.ps1'

Copy link
Owner Author

Choose a reason for hiding this comment

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

I considered that, but for windows we currently build with an additional platform matrix, so I wasn't sure how we should deal with that. Probably the easiest way would be to drop support for 32 bit windows builds, but its quite nice being able to build for the original architecture

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave 32-bit builds enabled, if not for checking our pointer arithmetic is correct.

You mean the ilammy/msvc-dev-cmd@v1.4.1 step? That can be made conditional on the os using

      - uses: ilammy/msvc-dev-cmd@v1.4.1
        if: contains(matrix.os, 'windows')
        with:
          arch: ${{ matrix.platform }}

Copy link
Collaborator

Choose a reason for hiding this comment

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

And the platforms can then only be added to the Windows items.

    include:
      - os: 'ubuntu-latest'
        script: '.github/scripts/build-linux.sh'
      - os: 'macos-latest'
        script: '.github/scripts/build-macos.sh'
      - os: 'windows-latest'
        script: '.github\\scripts\\build-msvc.ps1'
        platform: 'x86'
      - os: 'windows-latest'
        script: '.github\\scripts\\build-msvc.ps1'
        platform: 'amd64'

Copy link
Owner Author

Choose a reason for hiding this comment

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

no, I mean for windows we build x86 and x64. Ah, just saw your other comment, yep that should owrk

Comment on lines 18 to 20
tar -czvf dethrace-${BUILD_TAG}-darwin-amd64.tar.gz build/dethrace

echo "::set-output name=filename::dethrace-${BUILD_TAG}-darwin-amd64.tar.gz"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something what bothers me with the current Linux release is that the tarball contains a build folder.
How about we create a dethrace-${BUILD_TAG}-darwin-amd64 directory, and tar that directory?

Suggested change
tar -czvf dethrace-${BUILD_TAG}-darwin-amd64.tar.gz build/dethrace
echo "::set-output name=filename::dethrace-${BUILD_TAG}-darwin-amd64.tar.gz"
releasename="dethrace-${BUILD_TAG}-darwin-amd64"
rm -rf "$releasename"
mkdir "$releasename"
cp build/dethrace "$releasename/dethrace"
tar -czvf "$releasename.tar.gz" "$releasename"
echo "::set-output name=filename::"$releasename.tar.gz"

Copy link
Collaborator

Choose a reason for hiding this comment

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

The same applies to the Macos release binary.
The Windows zip contains no directories.

@dethrace-labs dethrace-labs merged commit 2708c61 into main Oct 3, 2022
@dethrace-labs dethrace-labs deleted the workflowtest3 branch October 3, 2022 03:45
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