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

Adding github action for Visual Studio #1234

Closed
wants to merge 1 commit into from

Conversation

mcuee
Copy link
Member

@mcuee mcuee commented Jan 24, 2023

This PR adds github action for Visual Studio.

Together with the existing MSYS2 github action, this wil address #1045.

@mcuee mcuee added the build_ci Build system, CI label Jan 24, 2023
@tormodvolden
Copy link
Contributor

This is probably fine, but do we need both this and appveyor? I am already uncomfortable that a tiny comment typo push causes such a waste of resources :) Appveyor is creating build artifacts archives, that would be great to improve to generate full binary packages. Is this possible with GitHub actions?

@tormodvolden
Copy link
Contributor

Maybe we could configure appveyor to build only on tags, and let this github action (it does indeed look a lot more simple and elegant) do the per-commit build.

@mcuee
Copy link
Member Author

mcuee commented Jan 24, 2023

Appveyor is creating build artifacts archives, that would be great to improve to generate full binary packages. Is this possible with GitHub actions?

Yes github actions can keep the artifcates.

Examples:
https://github.com/avrdudes/avrdude/blob/main/.github/workflows/build.yml (for all buids, binary and cmake build files)
https://github.com/libusb/hidapi/blob/master/.github/workflows/builds.yml (only for Windows msbuild)

@mcuee
Copy link
Member Author

mcuee commented Jan 24, 2023

Maybe we could configure appveyor to build only on tags, and let this github action (it does indeed look a lot more simple and elegant) do the per-commit build.

I think this is a good idea.

We can also improve github action build scripts later to keep the artifacts and remove appveyor altogether in the future.

@mcuee
Copy link
Member Author

mcuee commented Feb 11, 2023

Maybe we could configure appveyor to build only on tags, and let this github action (it does indeed look a lot more simple and elegant) do the per-commit build.

Ah, Appveyor has Cygwin build now -- so we may need to add cygwin github action first before going with this idea.

.github/workflows/msvc.yml Outdated Show resolved Hide resolved
@mcuee
Copy link
Member Author

mcuee commented May 8, 2023

@Youw

Your suggestion is good. Just wondering if you can help to suggest how to add the feature to keep the binary for MSVC build. Thanks.

@mcuee
Copy link
Member Author

mcuee commented May 8, 2023

@Youw
Hmm, strange, it says there is a syntax error which I can not figure out.

@Youw
Copy link
Member

Youw commented May 9, 2023

syntax error

Fixed. Coding inside of the suggestion box is tricky.

@Youw
Copy link
Member

Youw commented May 9, 2023

how to add the feature to keep the binary for MSVC build

Lets start with the general theory a bit: we want to store some binaries (probably right here on Github) which are produced by the build.
To store a binary, we need a storage, and when in comes to storage, we should ask what kind of storage do we want:

  • lifetime: temporary/permanent;
  • scope: generic for a repo? build? bound for some other entity?

NOTE: Github doesn't provide a generic repo/organisation-level storage (but there used to be - obsolete).

The only permanent storage there is on Github - an artifacts attached to a release. I don't think that is what we need right now.

The only available alternative - build artifacts bound to a specific build: https://github.com/marketplace/actions/upload-a-build-artifact
Example used by HIDAPI: https://github.com/libusb/hidapi/blob/b516e970480a8d05b1c392d106ca110a9d06d668/.github/workflows/builds.yml#L313
Available here: https://github.com/libusb/hidapi/actions/runs/4891807115

But that storage is always temporary (see Retention Period).

.github/workflows/msvc.yml Outdated Show resolved Hide resolved
@mcuee
Copy link
Member Author

mcuee commented May 9, 2023

I think for the purpose of this PR, we just need to keep the binary files for a specific build. I understand it is a temporary storage.

@mcuee
Copy link
Member Author

mcuee commented May 17, 2023

@tormodvolden

I think we can merge this PR first. Later we can discuss whether we want to keep the binaries or not.

@tormodvolden
Copy link
Contributor

Can you please squash all the fixup commits?

@tormodvolden
Copy link
Contributor

And if it cannot store artifacts, does it have any advantage over the appveyor builds?

@Youw
Copy link
Member

Youw commented May 17, 2023

if it cannot store artifacts

Oh it can (see here), it is just a matter of getting it done.

any advantage over the appveyor builds?

At least one thing - all in one place. The build, the logs, etc. - all at github and no additional integration is needed.

But there is a drawback: this build actually checks a lot less than the Appveyor version (only Latest Windows/Visual Studio and only a single (latest) toolset).

@tormodvolden
Copy link
Contributor

all in one place

Could seem like a pragmatic advantage. On a general note though, if we all stop supporting the alternatives we may well be finding ourselves without alternatives one day.

@Youw
Copy link
Member

Youw commented May 17, 2023

a pragmatic advantage

Just a convenience I'd say.


Regarding the alternatives note. This doesn't seem like a replacement for Appveyor builds - only additional alternative.

BTW: @mcuee what is the original reason to have this? As a potential way to store automatic artifacts, or is there other reason?

@mcuee
Copy link
Member Author

mcuee commented May 18, 2023

BTW: @mcuee what is the original reason to have this? As a potential way to store automatic artifacts, or is there other reason?

The idea is to have github action for Windows. To me it is faster than Appveyor.

Then the next step is to keep the artifacts, that is not the original intention of the PR, but rather the next step.

@mcuee
Copy link
Member Author

mcuee commented May 18, 2023

Can you please squash all the fixup commits?

@Youw
Can you help here? I am not good at git and I am afraid that I may screw up the PR.

@tormodvolden
Copy link
Contributor

tormodvolden commented May 18, 2023

To make sure you are not screwing up anything while rebasing, just check out a branch copy to work on:
(note what the original branch is called, e.g. my_master)
git checkout -b new_b
Now rebase it on top of master:
git rebase -i origin/master
In the editor you can now replace "pick" with "s" (for squash) on the second and further commits. And clean up the commit message.
Afterwards you can compare this fixed-up branch with the orginal:
git diff my_master
Or, since many other files were changed since then:
git diff my_master -- .github/
If everything is fine, you can force-push the new branch directly, or maybe easier without having to know the remote name etc, update your original branch and repush it:
git checkout my_master
git reset --hard new_b
git push -f

@mcuee
Copy link
Member Author

mcuee commented May 21, 2023

@tormodvolden

Thanks for the great help. I have tried your steps and it seems to work. Hope I am doing it correctly.

The first step -- I am not so sure what to do so I actually created the new_b branch from the github Web interface using the github_action as the base.

git clone https://github.com/mcuee/libusb.git -b new_b
cd .\libusb\
git rebase -i origin/master (replace "pick" with "s" (for squash) on the second and further commits. And clean up the commit message.)
git diff github_action
 git checkout github_action
git reset --hard new_b
git push -f

@mcuee
Copy link
Member Author

mcuee commented Oct 19, 2023

@tormodvolden

Just wondering if you are okay with this PR now. Thanks.

@mcuee mcuee mentioned this pull request Oct 19, 2023
@tormodvolden
Copy link
Contributor

What is the situation about the artifacts, and the duplication of AppVeyor?

@mcuee
Copy link
Member Author

mcuee commented Oct 19, 2023

What is the situation about the artifacts, and the duplication of AppVeyor?

Hmm, I think we can keep AppVeyor and the artifacts generated by AppVeyor.

github action is faster and we can have future PRs to improve this and then totally replace AppVeyor (eg: support more versions of MSVC, keeping the artifacts). But I do not know how to do that myself.

- uses: actions/checkout@v3
- uses: microsoft/setup-msbuild@v1.1
- name: MSBuild ${{ matrix.platform }}
run: msbuild msvc\libusb.sln /p:Configuration=Release /p:Platform=${{ matrix.platform }}
Copy link
Contributor

Choose a reason for hiding this comment

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

should use msbuild-architecture instead.

build:
runs-on: windows-latest
strategy:
matrix:
Copy link
Contributor

Choose a reason for hiding this comment

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

AppVeyor tests various versions of MSVC. You should use vs-version.

@mcuee
Copy link
Member Author

mcuee commented Oct 20, 2023

I think I will close this PR first.

@neheb
You seem to be familiar with this topic. PR is welcome.

@mcuee mcuee closed this Oct 20, 2023
@mcuee mcuee deleted the github_action branch October 20, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build_ci Build system, CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants