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.

Sign up for GitHub

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

Jump to bottom

feat!: build with CMake and release with semantic-release #118

Merged
merged 6 commits into from
Nov 15, 2023
Merged

feat!: build with CMake and release with semantic-release #118

merged 6 commits into from
Nov 15, 2023

Conversation

dsanders11
Copy link
Member

@dsanders11 dsanders11 commented Nov 8, 2023 •
edited
Loading

  • The original gyp build in this repo has broken - since gyp is more or less unmaintained these days, switch to CMake
    • I tried to replicate the gyp config in CMake
  • Move from AppVeyor to GitHub Actions - it will be simpler to maintain going forward
  • Refactor how the version is determined for printing help - rather than hardcoding it in version.h
    • By fetching it from the product version, we can dogfood and stamp the version into the exe after building
  • Use semantic-release to create GitHub releases
    • Paired with the refactor, we can stamp the correct version into the exe at build time with no manual work
  • Drops support for Windows XP
    • Visual Studio 2017 deprecated support and CI is on Visual Studio 2022, so it's not feasible to continue supporting it

@dsanders11 dsanders11 force-pushed the ci-github-actions branch 13 times, most recently from 6cfdefd to 80c33de Compare November 9, 2023 08:54
@dsanders11 dsanders11 changed the title ci: use GitHub Actions refactor!: build with CMake and release with semantic-release Nov 9, 2023
@dsanders11 dsanders11 force-pushed the ci-github-actions branch 6 times, most recently from 53a07d8 to ced98af Compare November 9, 2023 22:18
@dsanders11 dsanders11 changed the title refactor!: build with CMake and release with semantic-release feat!: build with CMake and release with semantic-release Nov 9, 2023
@dsanders11
feat!: build with CMake and release with semantic-release
1d4b906 
BREAKING CHANGE: Drops Windows XP support
zcbenz
zcbenz reviewed Nov 9, 2023
View reviewed changes
Copy link
Contributor

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

You can store the data in a std::vector to avoid manual new/delete.

src/main.cc Outdated Show resolved Hide resolved
zcbenz
zcbenz reviewed Nov 9, 2023
View reviewed changes
src/main.cc Outdated Show resolved Hide resolved
zcbenz
zcbenz approved these changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

@dsanders11 dsanders11 marked this pull request as ready for review November 10, 2023 01:12
@dsanders11 dsanders11 requested a review from a team as a code owner November 10, 2023 01:12
VerteDinde
VerteDinde approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

LGTM - we should disable AppVeyor for this repo and kill that check

@dsanders11 dsanders11 merged commit b1f8a3b into master Nov 15, 2023
4 of 6 checks passed
@dsanders11 dsanders11 deleted the ci-github-actions branch November 15, 2023 22:03
This was referenced Nov 15, 2023
Closed
Closed
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz approved these changes

@VerteDinde VerteDinde VerteDinde approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dsanders11 @zcbenz @VerteDinde