Skip to content

ROCm packages: remove build type variants#39089

Merged
haampie merged 1 commit intospack:developfrom
boomanaiden154:llvm-amdgpu-build-type
Jul 27, 2023
Merged

ROCm packages: remove build type variants#39089
haampie merged 1 commit intospack:developfrom
boomanaiden154:llvm-amdgpu-build-type

Conversation

@boomanaiden154
Copy link
Copy Markdown
Contributor

@boomanaiden154 boomanaiden154 commented Jul 25, 2023

After #36679, the default build type is Release, so there is no need to explicitly set the build type as a custom variant.

@boomanaiden154
ROCm packages: remove build type variants
ad23f76 
After spack#36679, the default build type is Release, so there is no need to
explicitly set the build type as a custom variant.
@haampie
Copy link
Copy Markdown
Member

haampie commented Jul 26, 2023

I guess this is fine, but I can imagine that these packages actually only support a subset of the values.

@cgmb
Copy link
Copy Markdown
Contributor

cgmb commented Jul 26, 2023

I guess this is fine, but I can imagine that these packages actually only support a subset of the values.

IIRC, the main reason this was added originally was because the upstream projects defaulted to Release mode and they would sometimes be too large to link if built in their default configuration with debug symbols enabled.

@boomanaiden154
Copy link
Copy Markdown
Contributor Author

boomanaiden154 commented Jul 26, 2023

I guess this is fine, but I can imagine that these packages actually only support a subset of the values.

The only additional value in the CMake class is MinSizeRel which might be supported on all of these packages (definitely at least some), but I haven't check yet. I was originally planning on making much more sweeping changes because it seems like there were quite a few packages that seemingly haven't been updated yet, but I don't have infrastructure to test all the configurations, so I settled on just working on these packages. I'm also guessing there are quite a few packages currently not explicitly setting their build type options that would be broken if someone tried to build everything with MinSizeRel for example.

@haampie
Copy link
Copy Markdown
Member

haampie commented Jul 27, 2023

In that case, let's merge 👍

@haampie haampie merged commit 936c604 into spack:develop Jul 27, 2023
mpokorny pushed a commit to mpokorny/spack that referenced this pull request Sep 18, 2023
@boomanaiden154 @mpokorny
ROCm packages: remove build type variants (spack#39089)
809eb15 
After spack#36679, the default build type is Release, so there is no need to
explicitly set the build type as a custom variant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haampie haampie haampie approved these changes

@arjun-raj-kuppala arjun-raj-kuppala Awaiting requested review from arjun-raj-kuppala

@renjithravindrankannath renjithravindrankannath Awaiting requested review from renjithravindrankannath

@srekolam srekolam Awaiting requested review from srekolam

+1 more reviewer

@cgmb cgmb cgmb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

update-package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@boomanaiden154 @haampie @cgmb