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

Add back support for relocatable packages #2431

Merged
merged 1 commit into from
Aug 17, 2024

Conversation

mrexodia
Copy link
Contributor

@mrexodia mrexodia commented Aug 8, 2024

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

The previous PR #2134 not only 'allowed' an absolute CMAKE_INSTALL_LIBDIR/CMAKE_INSTALL_INCLUDEDIR, it also broke support for relocatable packages if the path was not absolute.

These changes should also be cherry-picked in v5.

Discussion: NixOS/nixpkgs#144170

@mrexodia mrexodia force-pushed the relocatable-package branch from 3794b56 to de35bb5 Compare August 8, 2024 23:12
@mrexodia mrexodia changed the base branch from v5 to next August 8, 2024 23:12
@Rot127 Rot127 added the build & packaging Build system and packaging related label Aug 10, 2024
@Rot127 Rot127 added this to the v6 milestone Aug 10, 2024
@Rot127
Copy link
Collaborator

Rot127 commented Aug 16, 2024

These changes should also be cherry-picked in v5.

Could you please open a PR with it?
We might even get it into the v5.0.2 path release today or tomorrow.

Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

In general looks good to me. Can you tell me how quickly this becomes "tested" via NixOS?

@Rot127
Copy link
Collaborator

Rot127 commented Aug 16, 2024

@kabeor Why is the CI not running here?

@mrexodia mrexodia mentioned this pull request Aug 16, 2024
@mrexodia
Copy link
Contributor Author

In general looks good to me. Can you tell me how quickly this becomes "tested" via NixOS?

No idea, I do not use NixOS. They are doing something different that is specifically recommended against by CMake's documentation. This patch goes back to the behavior before #2134, unless those paths are absolute (as will be the case with NixOS). It's silly this workaround is required, but such is life 🤷🏻‍♂️

@kabeor
Copy link
Member

kabeor commented Aug 16, 2024

Really wired, may be affected by github downtime? Can we commit something or open a new pr to see if CI come back?

@mrexodia
Copy link
Contributor Author

This one also doesn't run CI: #2447

@XVilka
Copy link
Contributor

XVilka commented Aug 17, 2024

If CI doesn't run, it might be the sign of some error in the Actions YAML file. Check "Actions" log of the repository.
Note also an error of using outdated NodeJS runner: https://github.com/capstone-engine/capstone/actions/runs/10419892239

@Rot127
Copy link
Collaborator

Rot127 commented Aug 17, 2024

@XVilka The yaml files are fine. The CI also runs for the other PRs as well.
@mrexodia Can you rebase this one? The labeler was at least working on the v5 PR. Maybe it is indeed a Github outage problem.

@mrexodia mrexodia force-pushed the relocatable-package branch from de35bb5 to 7f87c20 Compare August 17, 2024 11:05
@mrexodia
Copy link
Contributor Author

Done!

@Rot127
Copy link
Collaborator

Rot127 commented Aug 17, 2024

For me all of the checks are green now.
@kabeor Please take a look.

Copy link
Member

@kabeor kabeor left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks.

@kabeor kabeor merged commit f38d56b into capstone-engine:next Aug 17, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build & packaging Build system and packaging related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants