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

Only link library specific files #84

Merged
merged 10 commits into from
Apr 6, 2022
Merged

Conversation

lawruble13
Copy link
Collaborator

This is not compatible with CMake 3.10 (due to the use of generator expressions in install(CODE), requiring the setting of CMP0087). Can anyone think of a better method to do the same thing?

Copy link
Contributor

@TorreZuk TorreZuk left a comment

Choose a reason for hiding this comment

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

We require cmake 3.16.8 for rocblas so hope that 3.10 is fine, don't know if we have any requirement on minimum version.
If you fix failing test, then maybe you can try in the docker as not clear by the bin/ folder was being referenced in the failure ticket and this is still only lib/ directory correct so perhaps the bin contents need symlinks from old install locations as well?

Copy link
Collaborator

@cgmb cgmb left a comment

Choose a reason for hiding this comment

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

Perhaps you can get the name from get_target_property? I'm not sure which that TARGET_LINKER_FILE_NAME corresponds to, but I imagine it can be found somewhere in the list of target properties.

share/rocm/cmake/ROCMInstallTargets.cmake Outdated Show resolved Hide resolved
@lawruble13
Copy link
Collaborator Author

Perhaps you can get the name from get_target_property? I'm not sure which that TARGET_LINKER_FILE_NAME corresponds to, but I imagine it can be found somewhere in the list of target properties.

I had looked, and did so again, to check if any of these properties could be used. Most of the relevant ones are not set, the closest solution I found to get the file name without generator expressions was using the LOCATION property, which was deprecated with 3.0.

The current solution generates a temporary script, which is permitted to use generator expressions, and install(SCRIPT) to run it. Not ideal, and I'm happy to hear suggestions of how to improve it.

@cgmb
Copy link
Collaborator

cgmb commented Mar 31, 2022

If using the temporary script works, I have no real objections to that approach.

@lawruble13 lawruble13 marked this pull request as ready for review April 1, 2022 18:18
@TorreZuk
Copy link
Contributor

TorreZuk commented Apr 4, 2022

Why are we running macOS tests? @pfultz2 @saadrahim should they block Liam

@pfultz2
Copy link
Collaborator

pfultz2 commented Apr 4, 2022

Why are we running macOS tests?

We should make sure rocm-cmake works on all platforms. If a component needs to support a platform(such as mac os) in the future then rocm-cmake should not be a blocker.

should they block Liam

The error is from windows not mac os(which was just canceled due to error on windows). It looks like the windows-2016 image is no longer available, we need to switch to windows-2022.

@pfultz2
Copy link
Collaborator

pfultz2 commented Apr 4, 2022

This line here needs to be updated with windows-2022:

https://github.com/RadeonOpenCompute/rocm-cmake/blob/master/.github/workflows/test.yml#L36

I can open a seperate PR to update it or you can update it directly in this PR, whichever is easier for you.

@lawruble13
Copy link
Collaborator Author

I updated the noted line, however CMake seems to be unable to locate cl on the new version.

@lawruble13
Copy link
Collaborator Author

Using windows-2019 instead, as VS2022 is not yet supported, and the windows-2022 image does not have the Windows Driver Kit.

@lawruble13
Copy link
Collaborator Author

lawruble13 commented Apr 4, 2022

CMake 3.10 is not aware of and does not support Visual Studio 2019, which is the version of Visual Studio in windows-2019. The earliest version which does is CMake 3.14. @pfultz2 preferred resolution?

@pfultz2
Copy link
Collaborator

pfultz2 commented Apr 4, 2022

@pfultz2
Copy link
Collaborator

pfultz2 commented Apr 4, 2022

Let me adjust the required statuses so this can be merged.

@lawruble13
Copy link
Collaborator Author

Any idea why the macos tests aren't running?

@TorreZuk
Copy link
Contributor

TorreZuk commented Apr 5, 2022

Any idea why the macos tests aren't running?

No clue on the test queue, but I would at least vote to make the macos tests not required, are you ready to merge otherwise? I don't even support running them until we actually have use cases on macos. This fix is blocking downstream builds so hope it can get in today if you are happy with it so we can iterate if it causes some new issues. @DanyiLin has a windows build pipeline consuming rocm-cmake so will be interesting to see if this will trigger it.

@cgmb
Copy link
Collaborator

cgmb commented Apr 6, 2022

Perhaps try bumping it to macOS-10.15? I'm not certain where to find the supported platforms list for GitHub Actions, but macOS 10.14 isn't in Azure's list of Microsoft-hosted agents.

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.

5 participants