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

Revert "Merge pull request #8072 from rizlik/github-fix" #8074

Merged
merged 9 commits into from
Oct 29, 2024

Conversation

bandi13
Copy link
Contributor

@bandi13 bandi13 commented Oct 15, 2024

This reverts commit 0f8b4db, reversing changes made to 743a78d.

This reverts commit 0f8b4db, reversing
changes made to 743a78d.
@bandi13 bandi13 self-assigned this Oct 15, 2024
@bandi13
Copy link
Contributor Author

bandi13 commented Oct 22, 2024

Depends on wolfSSL/osp#205.

@bandi13 bandi13 assigned wolfSSL-Bot and unassigned bandi13 Oct 22, 2024
@bandi13
Copy link
Contributor Author

bandi13 commented Oct 22, 2024

Note that jwt-cpp tests pass if using the PR of wolfSSL/osp#205.

ref: [ 0.6.0 ]
ref: [ 0.7.0 ]
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not testing 0.6.0 anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0.6.0 does not compile on the latest compiler because of a build error.

Copy link
Member

Choose a reason for hiding this comment

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

We should fix that error. What is the error?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the example you gave uses the ubuntu-22.04 runner. It will fail on ubuntu-latest. See: https://github.com/wolfSSL/wolfssl/actions/runs/11350595837/job/31569431321.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't want to invest time investigating the source of the error, we should at least run those tests on the version of Ubuntu it passes on. You can add the runner choice to the strategy to run 0.6 on the lts version and 0.7 on latest.

Copy link
Contributor Author

@bandi13 bandi13 Oct 23, 2024

Choose a reason for hiding this comment

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

It's not that I don't want to investigate the source of the error. It's simply adding in #include <cstdint.h> into base.h, which is what jwt-cpp has already done. I have done as you suggested and just added a matrix.

Comment on lines 15 to 16
jobs:
build_wolfssl:
name: Build wolfSSL
# Just to keep it the same as the testing target
runs-on: ubuntu-22.04
# This should be a safe limit for the tests to run.
timeout-minutes: 4
steps:
- name: Build wolfSSL
uses: wolfSSL/actions-build-autotools-project@v1
with:
path: wolfssl
configure: --enable-all CFLAGS=-DWOLFSSL_NO_ASN_STRICT
install: true
check: false

- name: tar build-dir
run: tar -zcf build-dir.tgz build-dir

- name: Upload built lib
uses: actions/upload-artifact@v4
with:
name: wolf-install-sssd
path: build-dir.tgz
retention-days: 5

sssd_check:
Copy link
Member

Choose a reason for hiding this comment

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

Why are we including building wolfSSL in the sssd_check job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just consolidating the job to be simpler. I didn't see a need to upload an artifact only to download it again. I could change the name of the stage if that's better.

Copy link
Member

Choose a reason for hiding this comment

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

It simplifies the process of adding a new version so that testing can be done in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but we're only testing a single version so it seemed superfluous. I'll revert this change.

Copy link
Member

Choose a reason for hiding this comment

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

It decreases the workload on the person updating the support. I think its worth it to separate on the initial write.

@julek-wolfssl julek-wolfssl assigned bandi13 and unassigned wolfSSL-Bot Oct 23, 2024
@bandi13 bandi13 assigned wolfSSL-Bot and unassigned bandi13 Oct 23, 2024
@douzzer douzzer merged commit 0ded8ba into wolfSSL:master Oct 29, 2024
142 checks passed
@bandi13 bandi13 deleted the revertGithubFix branch October 29, 2024 20:50
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.

4 participants