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

[HIPIFY][build][install][fix] Pass USE_SOURCE_PERMISSIONS when installing scripts #1841

Open
wants to merge 1 commit into
base: rocm-6.3.x
Choose a base branch
from

Conversation

stellaraccident
Copy link

Pass USE_SOURCE_PERMISSIONS when installing scripts.

Without this, hipify-perl and friends are installed with non-executable permissions, requiring after the fact fixup.

Without this, hipify-perl and friends are installed with non-executable permissions, requiring after the fact fixup.
@emankov emankov added build Building issue/fix fix It fixes bug labels Jan 22, 2025
@emankov emankov changed the title Pass USE_SOURCE_PERMISSIONS when installing scripts. [HIPIFY][build][install][fix] Pass USE_SOURCE_PERMISSIONS when installing scripts Jan 22, 2025
@emankov emankov changed the title [HIPIFY][build][install][fix] Pass USE_SOURCE_PERMISSIONS when installing scripts [HIPIFY][build][install][fix] Pass USE_SOURCE_PERMISSIONS when installing scripts Jan 22, 2025
Copy link
Collaborator

@emankov emankov left a comment

Choose a reason for hiding this comment

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

Please rebase - your PR should only include your commit(s) related to the PR w/o any merges and others' commits.

CMakeLists.txt Outdated Show resolved Hide resolved
@stellaraccident stellaraccident changed the base branch from amd-staging to rocm-6.3.x January 22, 2025 15:41
@stellaraccident
Copy link
Author

Ptal

Copy link
Collaborator

@emankov emankov left a comment

Choose a reason for hiding this comment

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

LGTM

@emankov emankov requested a review from raramakr January 22, 2025 17:55
@emankov
Copy link
Collaborator

emankov commented Jan 22, 2025

@raramakr, could you have a look, too? Thanks.

Copy link
Contributor

@raramakr raramakr left a comment

Choose a reason for hiding this comment

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

For installing shell scripts cmake recommends to use install( PROGRAMS ..) as in line number 194-195 of CMakeLists.txt

(https://cmake.org/cmake/help/latest/command/install.html#programs)
The PROGRAMS form is identical to the FILES form except that the default permissions for the installed file also include OWNER_EXECUTE, GROUP_EXECUTE, and WORLD_EXECUTE. This form is intended to install programs that are not targets, such as shell scripts. Use the TARGETS form to install targets built within the project.

@stellaraccident
Copy link
Author

For installing shell scripts cmake recommends to use install( PROGRAMS ..) as in line number 194-195 of CMakeLists.txt

(https://cmake.org/cmake/help/latest/command/install.html#programs) The PROGRAMS form is identical to the FILES form except that the default permissions for the installed file also include OWNER_EXECUTE, GROUP_EXECUTE, and WORLD_EXECUTE. This form is intended to install programs that are not targets, such as shell scripts. Use the TARGETS form to install targets built within the project.

Indeed - that explains the itch I was having because I hadn't encountered this before (I usually use one of the more recommended forms). I can try this change in my project when I get back through my patch queue. Also fine if someone else wants to land a bug fix.

@emankov
Copy link
Collaborator

emankov commented Jan 27, 2025

Hi @stellaraccident,

First, thanks for your contribution.
Second, are you going to incorporate hipify-perl into the existing install(PROGRAMS..., or do you want us to do it?

Thanks!

@stellaraccident
Copy link
Author

I've got a growing set of patches and would accept the help in taking this over, if you're offering. It is a bit of a pain to rebase and test in the rig I have.

@searlmc1
Copy link
Collaborator

Hi @stellaraccident ,

Is it critical to land this in the ROCm:rocm-6.3.x branch ? If so, we need to coordinate with ROCm-devOps as the source-of-truth for that branch is internal. Also, I assume this should land in the amd-staging branch. E.g., I assume this issue also exists in amd-staging / also needs to be fixed there.

Thanks much

@stellaraccident
Copy link
Author

No it is not required to land on 6.3.x. That is just where the diffbase originated from. Aakash (not sure of his GH id) is picking all of these patches from me up and working on getting them onto mainline (and working on getting our build rebased on mainline).

@searlmc1
Copy link
Collaborator

OK ; sounds good; they should land in staging first ; staging gets promoted to mainline . Between you | me | Evgeny | Aakash , we can make sure they land in the right places. fwiw: Aakash is @amd-aakash ; as Evgeny said, thanks for the patches .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Building issue/fix fix It fixes bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants