ROCm/rocblas
SHA256
7
0
Fork 3
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Make make rocblas build on SUSE fast #1

Closed
eeich wants to merge 0 commits from eeich/rocblas:main into main
pull from: eeich/rocblas:main
merge into: ROCm:main
Conversation 6 Commits - Files Changed 14 +600312 -304
eeich commented 2025-06-30 19:09:05 +02:00
Owner
Copy Link

This series of patches fixes the build for SUSE, splits it between
the core library and tensile module builds and restructures the spec
file.

This series of patches fixes the build for SUSE, splits it between the core library and tensile module builds and restructures the spec file.
autogits-devel requested review from mslacken 2025-06-30 19:09:45 +02:00
autogits-devel requested review from patrikjakobsson 2025-06-30 19:09:49 +02:00
autogits-devel requested review from rguenther 2025-06-30 19:09:51 +02:00
autogits-devel requested review from trix 2025-06-30 19:09:56 +02:00
eeich requested review from Owners 2025-06-30 20:20:35 +02:00
trix commented 2025-06-30 20:44:52 +02:00
Owner
Copy Link

Is flavor/mulibuild necessary if only 1 thing is built ?
There are looping constructs added here that are not needed, simplify loops down to a single line.
ex
%(for i in sed -ne "/package/s@ *<package>\([^<]*\)</package>@\1@p" %{S:2};
do echo "Requires: rocblas-tensile-$i = %version";
done)
I was going to ask that this sed magic be commented, but that isn't needed when this is a single line.

Is flavor a good variable name ? No one on the fedora side is going to know what that means.
The variable use_flavors is defined but never used.

if flavor is never going to work with testing, the disable test by something like
%if %{with flavor}
%bcond_with test
Similar for other options like 950

separate out suse setting of the gpu_list, this will be a high change area
casual or non suse users are not going to know what this means.
%if 0%{!?_flavor:1} || "%{?_flavor}" == "all"

similar separate suse, then flavor conditions from things like
ex/
%if 0%{!?_flavor:1}
%files
%license LICENSE.md

So it is easy for the fedora/rhel people see where their changes need to be.
Patch3 is not in this commit

Is flavor/mulibuild necessary if only 1 thing is built ? There are looping constructs added here that are not needed, simplify loops down to a single line. ex %(for i in `sed -ne "/package/s@ *<package>\([^<]*\)</package>@\1@p" %{S:2}`;\ do echo "Requires: rocblas-tensile-$i = %version";\ done) I was going to ask that this sed magic be commented, but that isn't needed when this is a single line. Is flavor a good variable name ? No one on the fedora side is going to know what that means. The variable use_flavors is defined but never used. if flavor is never going to work with testing, the disable test by something like %if %{with flavor} %bcond_with test Similar for other options like 950 separate out suse setting of the gpu_list, this will be a high change area casual or non suse users are not going to know what this means. %if 0%{!?_flavor:1} || "%{?_flavor}" == "all" similar separate suse, then flavor conditions from things like ex/ %if 0%{!?_flavor:1} %files %license LICENSE.md So it is easy for the fedora/rhel people see where their changes need to be. Patch3 is not in this commit
eeich commented 2025-06-30 21:27:10 +02:00
Author
Owner
Copy Link

Is flavor/mulibuild necessary if only 1 thing is built ?

Yes, because 2 things are built:

  • the default - i.e. where no flavor is mentioned.
  • the flavor 'all'

Without _multibuild, only one build would take place.

There are looping constructs added here that are not needed, simplify loops down to a single line.
ex
%(for i in sed -ne "/package/s@ *<package>\([^<]*\)</package>@\1@p" %{S:2};
do echo "Requires: rocblas-tensile-$i = %version";
done)
I was going to ask that this sed magic be commented, but that isn't needed when this is a single line.

I can simplify this. The loop is not required if there is only one flavor.

Is flavor a good variable name ? No one on the fedora side is going to know what that means.

This line is needed:

%global flavor @BUILD_FLAVOR@%{?nil}

as @BUILD_FLAVOR@ is replaced by OBS.
You can replace 'flavor' by another term if you have a better idea.

The variable use_flavors is defined but never used.

Not true. It is used 3 times.

if flavor is never going to work with testing, the disable test by something like
%if %{with flavor}
%bcond_with test

It is, at least I've tried to fix it. I haven't tested as I wasn't able to use the machine with the AMD GPU.

Similar for other options like 950

I'll check this.

separate out suse setting of the gpu_list, this will be a high change area
casual or non suse users are not going to know what this means.

Should we call it suse_flavor to make it clearer?

%if 0%{!?_flavor:1} || "%{?_flavor}" == "all"

I can simplify this if we are just having an all flavor.

similar separate suse, then flavor conditions from things like
ex/
%if 0%{!?_flavor:1}
%files
%license LICENSE.md

So it is easy for the fedora/rhel people see where their changes need to be.

This condition is true for fedora/rhel as well as when the 'default' flavor is used. I could do:

%if 0%{!?suse_version:1} || 0%{!?_flavor:1}
...
%endif

Not sure if this would be more readable.

Patch3 is not in this commit

This is because it is already in the sources from a previous commit. It never got removed.

> Is flavor/mulibuild necessary if only 1 thing is built ? Yes, because 2 things are built: - the default - i.e. where no flavor is mentioned. - the flavor 'all' Without _multibuild, only one build would take place. > There are looping constructs added here that are not needed, simplify loops down to a single line. > ex > %(for i in `sed -ne "/package/s@ *<package>\([^<]*\)</package>@\1@p" %{S:2}`;\ > do echo "Requires: rocblas-tensile-$i = %version";\ > done) > I was going to ask that this sed magic be commented, but that isn't needed when this is a single line. I can simplify this. The loop is not required if there is only one flavor. > Is flavor a good variable name ? No one on the fedora side is going to know what that means. This line is needed: ``` %global flavor @BUILD_FLAVOR@%{?nil} ``` as `@BUILD_FLAVOR@` is replaced by OBS. You can replace 'flavor' by another term if you have a better idea. > The variable use_flavors is defined but never used. Not true. It is used 3 times. > if flavor is never going to work with testing, the disable test by something like > %if %{with flavor} > %bcond_with test It is, at least I've tried to fix it. I haven't tested as I wasn't able to use the machine with the AMD GPU. > Similar for other options like 950 I'll check this. > separate out suse setting of the gpu_list, this will be a high change area > casual or non suse users are not going to know what this means. Should we call it `suse_flavor` to make it clearer? > %if 0%{!?_flavor:1} || "%{?_flavor}" == "all" I can simplify this if we are just having an `all` flavor. > similar separate suse, then flavor conditions from things like > ex/ > %if 0%{!?_flavor:1} > %files > %license LICENSE.md > So it is easy for the fedora/rhel people see where their changes need to be. This condition is true for fedora/rhel as well as when the 'default' flavor is used. I could do: ``` %if 0%{!?suse_version:1} || 0%{!?_flavor:1} ... %endif ``` Not sure if this would be more readable. > Patch3 is not in this commit This is because it is already in the sources from a previous commit. It never got removed.
eeich commented 2025-06-30 21:45:18 +02:00
Author
Owner
Copy Link

Similar for other options like 950

950 won't work with the spec file as shipped with suse - the %build script does:

module load rocm/gfx950

which requires Modules which we don't ship. We do ship Lmod on Tumbleweed, Leap and SLE15, but this has been dropped from SLES16 as we no longer support HPC.

> Similar for other options like 950 950 won't work with the spec file as shipped with suse - the `%build` script does: ``` module load rocm/gfx950 ``` which requires `Modules` which we don't ship. We do ship `Lmod` on Tumbleweed, Leap and SLE15, but this has been dropped from SLES16 as we no longer support HPC.
eeich commented 2025-06-30 21:54:40 +02:00
Author
Owner
Copy Link

This line is needed:

%global flavor @BUILD_FLAVOR@%{?nil}

as @BUILD_FLAVOR@ is replaced by OBS.
You can replace 'flavor' by another term if you have a better idea.

I can set a variable build_tensile_modules instead when @BUILD_FLAVOR@ is set to 'all' if you think this would be better.

> This line is needed: > ``` > %global flavor @BUILD_FLAVOR@%{?nil} > ``` > as `@BUILD_FLAVOR@` is replaced by OBS. > You can replace 'flavor' by another term if you have a better idea. I can set a variable build_tensile_modules instead when `@BUILD_FLAVOR@` is set to 'all' if you think this would be better.
trix commented 2025-06-30 22:38:56 +02:00
Owner
Copy Link

Try rpmbuild,

rpmbuild -ba rocblas.spec
error: line 282: Illegal char '@' (0x40) in: %package -n rocblas-tensile-@BUILD_FLAVOR@

Try rpmbuild, > rpmbuild -ba rocblas.spec error: line 282: Illegal char '@' (0x40) in: %package -n rocblas-tensile-@BUILD_FLAVOR@
eeich force-pushed main from d48013a128 to df12ff1591 2025-07-01 07:52:39 +02:00 Compare
eeich commented 2025-07-01 08:26:00 +02:00
Author
Owner
Copy Link

Try rpmbuild,

rpmbuild -ba rocblas.spec
error: line 282: Illegal char '@' (0x40) in: %package -n rocblas-tensile-@BUILD_FLAVOR@

This is not expected to work - at least not on systems that have %suse_version defined. You need to use the spec file from the source rpm which is 'preprocessed' already.
At least this one should be mitigated now with the update I've pushed now.
You may stumble over the next issue though where you are unable to expand %python_module.
The correct way to test build these is to use osc build <repository>.

> Try rpmbuild, > > rpmbuild -ba rocblas.spec > error: line 282: Illegal char '@' (0x40) in: %package -n rocblas-tensile-@BUILD_FLAVOR@ This is not expected to work - at least not on systems that have `%suse_version` defined. You need to use the spec file from the source rpm which is 'preprocessed' already. At least this one should be mitigated now with the update I've pushed now. You may stumble over the next issue though where you are unable to expand `%python_module`. The correct way to test build these is to use `osc build <repository>`.
eeich force-pushed main from df12ff1591 to c802f330a0 2025-07-01 08:52:30 +02:00 Compare
eeich force-pushed main from c802f330a0 to 8c95dc8f26 2025-07-02 08:00:04 +02:00 Compare
eeich force-pushed main from 8c95dc8f26 to a6091a9a26 2025-07-03 18:05:28 +02:00 Compare
eeich force-pushed main from a6091a9a26 to 216f9d4756 2025-07-03 18:09:38 +02:00 Compare
eeich closed this pull request 2025-08-29 14:49:10 +02:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
mslacken
patrikjakobsson
rguenther
trix
ROCm/Owners
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adamm autogits-devel eeich mslacken patrikjakobsson rguenther trix
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: ROCm/rocblas#1
Delete Branch "eeich/rocblas:main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?