-
Notifications
You must be signed in to change notification settings - Fork 9
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
SlowMerge Challenge #34
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look promising, was not aware that will merge and see test runs ...
@spreequalle: successfully verified that you could build ARM and ARM64 🎉 |
@spreequalle: While building looks great, I did not see any attempt to publish the package yet:
Has this step been disabled by intention? In order to win the SlowMerge Challenge, some minor steps are still missing:
❤️ |
if: startsWith(matrix.python-arch, 'arm') != true | ||
- name: Generating conan user directory and building the solution | ||
run: | | ||
conan user | ||
python build.py | ||
env: | ||
CONAN_REFERENCE: "ZenGitHub/1.0" | ||
CONAN_USERNAME: "jonico" | ||
CONAN_LOGIN_USERNAME: "jonico" | ||
CONAN_CHANNEL: "stable" | ||
CONAN_UPLOAD: "https://api.bintray.com/conan/conan-jonico/libzengithub" | ||
CONAN_STABLE_BRANCH_PATTERN: "release/*" | ||
CONAN_VISUAL_VERSIONS: ${{ matrix.vs-version }} | ||
CONAN_APPLE_CLANG_VERSIONS: "11.0" | ||
CONAN_GCC_VERSIONS: ${{ matrix.gcc-version }} | ||
CONAN_ARCHS: ${{ matrix.python-arch }} | ||
if: startsWith(matrix.python-arch, 'arm') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a lot of code duplication. Would the other environments break if you added the CONAN_ARCHS
env for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would break, since conan arch are not python arch, e.g. conan autodetect come up with both x86 and x86_64 for x86_64.
for arm it looks like it fails to detect and tries to build for x86 :(
https://github.com/spreequalle/libzengithub/runs/531279338?check_suite_focus=true
less code duplication, here
https://github.com/spreequalle/libzengithub/blob/master/.github/workflows/package.yml#L70
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about the upload though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
less code duplication, here
https://github.com/spreequalle/libzengithub/blob/master/.github/workflows/package.yml#L70
That looks good, would accept this in a FUP PR
- Address in-line feedback regarding code duplication in the workflow file
not sure about the upload though
Looking into the build log it appears as if the version of libzengithub referred to has already been cached:
ZenGitHub/1.0@jonico/stable:d286bc363f1ec1891f6eb391dbadf6cee665c357 - Cache
Could it be that you did not clean up your runner between builds and hence a previous upload attempt of what you tried to build failed, conan is not trying to do so again or is that checksum associated to a conan package I already uploaded for Linux?
conan search ZenGitHub/1.0@jonico/stable -r conan | grep d286bc363f1e did not reveal any results, so it is not referring to a conan package already uploaded ... |
i deleted the conan dirs and rebuild |
🎉 as the build log states:
I will take care of the credentials. Please attach your runners directly to jonico/libzengithub, you should have the permissions to do this now. |
|
* #34 solves essential tasks already * follow-up items discussed in PR
as promised, add working arm, arm64 images.