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

Trigger actions for PRs and pin the version of libcimpp #329

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

gnakti
Copy link
Contributor

@gnakti gnakti commented Dec 10, 2024

  • Since we work now mainly from forks, I extended the actions triggered on: push to be additionally triggered on: pull_request. Activity types set to types: [opened, synchronize, reopened]. For more details see here. An optimised version of this is to additionally prevent feature branches to trigger on push when they already have a PR: on: [push, pull_request]
  • Pin the version of libcimpp to avoid failure of windows workflow caused by sogno-platform/libcimpp#29.

Signed-off-by: Ghassen Nakti <ghassen.nakti@eonerc.rwth-aachen.de>
@leonardocarreras
Copy link
Collaborator

leonardocarreras commented Dec 10, 2024

It looks like there is a non-related problem (the Windows runner changed) that maybe is also possible to solve in the PR or in a different one, by explicitly installing this library just after the checkout process... In theory, the solution could be something like this...

    - name: Install dependencies (LibXml2)
      uses: microsoft/vcpkg-action@v2
      with:
        vcpkgArgs: install libxml2

and later, adding it to the path for cmake

        cmake -DWITH_PYBIND=OFF -DCMAKE_TOOLCHAIN_FILE=$VCPKG_ROOT/scripts/buildsystems/vcpkg.cmake ..

@gnakti
Copy link
Contributor Author

gnakti commented Dec 10, 2024

Thanks @leonardocarreras!
Better solve it in a new PR with a new branch.

@stv0g
Copy link
Contributor

stv0g commented Dec 10, 2024

Maybe the failing pipeline is caused by sogno-platform/libcimpp#29 ?

If thats the case, we should really pin the version of libcimpp which is used to avoid such breakage in the future.

/cc @m-mirz

@m-mirz
Copy link
Contributor

m-mirz commented Dec 10, 2024

Maybe the failing pipeline is caused by sogno-platform/libcimpp#29 ?

If thats the case, we should really pin the version of libcimpp which is used to avoid such breakage in the future.

/cc @m-mirz

@stv0g yes, the version should be pinned. I thought this was already the case. In this libcimpp PR we dropped support for every parser except libxml. So windows does not work out of the box but with @leonardocarreras suggestion it might work. Let me know if you think this change causes too many issues. Basically, I would like to get rid of the arabica library.

@gnakti gnakti changed the title Trigger actions for PRs Trigger actions for PRs and pin the version of libcimpp Dec 11, 2024
@gnakti gnakti marked this pull request as draft December 11, 2024 09:46
@gnakti
Copy link
Contributor Author

gnakti commented Dec 11, 2024

I edited the PR to pin the version of libcimpp. Let's do that first and then test the proposition of @leonardocarreras in a separate PR.

Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
…hen triggering in a fork

Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
@leonardocarreras
Copy link
Collaborator

I edited the PR to pin the version of libcimpp. Let's do that first and then test the proposition of @leonardocarreras in a separate PR.

@gnakti I added a pinning to the library for cmake and for the containers, and also fixed the sonarcloud execution by skipping gracefully when the secrets are not visible due to the package being triggered in a fork...

@leonardocarreras leonardocarreras marked this pull request as ready for review December 11, 2024 17:29
@leonardocarreras
Copy link
Collaborator

It runs in Windows too, I think we can consider it ready for review again @m-mirz @stv0g
Long term we will need to think when/if to implement the breaking changes

@m-mirz m-mirz merged commit 4a511b7 into sogno-platform:master Dec 11, 2024
17 checks passed
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