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.

Sign up for GitHub

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

Jump to bottom

[ICON] add variant extra-configure-args #896

Merged
merged 9 commits into from
Jan 4, 2024
Merged

[ICON] add variant extra-configure-args #896

merged 9 commits into from
Jan 4, 2024

Conversation

jonasjucker
Copy link
Contributor

@jonasjucker jonasjucker commented Jan 3, 2024 •
edited
Loading

Multi-value variant to inject any configure-argument not yet available as variant.
Ensure that format is either "--enable-arg" or "--disable-arg".

Solves #890

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 3, 2024 •
edited
Loading

PR Preview Action v1.4.6
Preview removed because the pull request was closed.
2024-01-04 14:26 UTC

@jonasjucker
Copy link
Contributor Author

launch jenkins icon

@jonasjucker jonasjucker linked an issue Jan 3, 2024 that may be closed by this pull request
[ICON] Add variant for extra configure args #890
Closed
@jenkins-apn
Copy link

tsa

🟢 unit test
Test
🟢summary
🔴 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🔴icon_extra_configure_args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec

WARNING: Serial tests did not run for system tests

@jenkins-apn
Copy link

balfrin

🟢 unit test
Test
🟢summary
🔴 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🔴icon_extra_configure_args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec

WARNING: Serial tests did not run for system tests

@jenkins-apn
Copy link

daint

🟢 unit test
Test
🟢summary
🔴 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🔴icon_extra_configure_args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec

WARNING: Serial tests did not run for system tests

@jonasjucker
Copy link
Contributor Author

launch jenkins icon

@jenkins-apn
Copy link

tsa

🟢 unit test
Test
🟢summary
🔴 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🔴icon_extra_config_args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec

WARNING: Serial tests did not run for system tests

@jenkins-apn
Copy link

balfrin

🟢 unit test
Test
🟢summary
🔴 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🔴icon_extra_config_args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec

WARNING: Serial tests did not run for system tests

@jenkins-apn
Copy link

daint

🟢 unit test
Test
🟢summary
🔴 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🔴icon_extra_config_args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec

WARNING: Serial tests did not run for system tests

@jonasjucker
Copy link
Contributor Author

launch jenkins icon

@jenkins-apn
Copy link

tsa

🟢 unit test
Test
🟢summary
🟢 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🟢icon_extra-config-args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec

@dominichofer
Copy link
Contributor

In my opinion this new variant has several design problems that need to be addressed.

  1. It's easy to get wrong: icon extra_config_args=--enable-new_faeture will be accepted without a problem, but the desired effect won't occur due to a spelling mistake.
  2. icon extra_config_args=--enable-aes and icon +aes will cause an equivalent configuration of icon, but not in the eyes of spack. Because the mechanics of spack are circumnavigated.
  3. This variant allows icon extra_config_args=--enable-cdi-pio, which would not add the necessary libs via libs += self.spec['libcdi-pio:fortran'].libs.
  4. icon extra_config_args=--enable-emvorado,--disable-mpi is permitted, even though icon +emvorado ~mpi is a conflict.
  5. The variant aes becomes redundant, as it is covered by extra_config_args=--enable-aes already.

@jenkins-apn
Copy link

balfrin

🟢 unit test
Test
🟢summary
🟢 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🟢icon_extra-config-args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec
🟢 system test
Test
🟢Icon-install_2_6_6_gcc
🟢Icon-install_2_6_6_nvhpc
🟢Icon-install_nwp_gpu

@jonasjucker
Copy link
Contributor Author

Good points.

Problem 2-5 could be prevented by only allowing values of extra-config-args to be non-variant values.
i.e. extra-config-args=--enable-aes would raise an error, because already a variant of the package.
I can implement a checker for that.

I think we can do only little about 1.
Spelling mistakes should be discovered by users itself.

Also I think we have to loosen a bit here in order to not releasing a new tag of spack-c2sm for each "pure" configure-arg. With "pure" I refer to option that do not need any additional dependencies etc.

@jonasjucker
Copy link
Contributor Author

Something like:

class Icon(AutotoolsPackage, CudaPackage):
    # ... (existing code)

    variant('extra_configure_arg',
            default=[],
            description='Additional configure arguments')

    def validate_extra_configure_arg(self):
        for extra_arg in self.spec.variants['extra_configure_arg'].value:
            if extra_arg.startswith(('--enable-', '--disable-')) and \
                    extra_arg[len('--enable-'):].split('=')[0] in self.variants:
                raise error.SpecError(f'The value "{extra_arg}" for the extra_configure_arg variant conflicts '
                                      f'with an existing variant. Choose a different value.')

    def configure_args(self):

        # Validate extra_configure_arg
        self.validate_extra_configure_arg()

@dominichofer
Copy link
Contributor

This doesn't fix the redundancy problem of aes. The variant can still be deleted and icon would build fine with extra_configure_args=--enable-aes.

But if a tight coupling is your concern, these variants would make it even looser icon extra_libs=libcdi-pio:fortran extra_dep=infero extra_compiler_flags=-O1,-g,--address-sanitizer omit_compiler_flags=-O3.
Of course this is a reduction ad absurdum and the point I'm trying to make is that this PR will only silence the problem for a while. Soon we're going to see the need for extra_compiler_flags and omit_compiler_flags because of debug flags and address- and memory-sanitizers and just like that we're going to end up with lots of complexity with string-parsers and regex. And spack wouldn't even be able to recognize these variants correctly since they are mushed strings.

@jenkins-apn
Copy link

daint

🟢 unit test
Test
🟢summary
🟢 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🟢icon_extra-config-args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec
🟢 system test
Test
🟢Icon-install_2_6_6_gcc
🟢daint_cpu_cce
🟢daint_cpu_gcc
🟢daint_cpu_nvhpc
🟢daint_cpu_nvhpc_out_of_source
🟢daint_gpu_nvhpc
🟢daint_dsl_nvhpc

@jonasjucker
Copy link
Contributor Author

launch jenkins icon

@jenkins-apn
Copy link

tsa

🟢 unit test
Test
🟢summary
🟢 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🟢icon_extra-config-args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec

@jenkins-apn
Copy link

balfrin

🟢 unit test
Test
🟢summary
🟢 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🟢icon_extra-config-args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec
🟢 system test
Test
🟢Icon-install_2_6_6_gcc
🟢Icon-install_2_6_6_nvhpc
🟢Icon-install_nwp_gpu

@jonasjucker
Copy link
Contributor Author

@dominichofer I see your point, but I think the scenario you sketch is unrealistic.
spack already provides option to inject compiler-flags through fcflags or cflag in spec.

It is a clear need from BuildBot to have a less tight coupling of spack-c2sm, i.e allowing the devs to have more freedom with the build.
The idea of this variant is that we gain more time until we have to release a new version of spack-c2sm, that then would translate all the extra_config_args into new variants.

We discussed this before X-Mas in the spack-meeting and it will make our lives easier.

I implemented now an additonal check and warnings that should make user aware if the potential damage this variant can have.

@dominichofer
Copy link
Contributor

@jonasjucker Thanks for the reminder, I forgot that we already discussed this. I have no further concerns.
Please ping me again when you're done editing the branch and the tests are green. I will review.

@jenkins-apn
Copy link

daint

🟢 unit test
Test
🟢summary
🟢 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🟢icon_extra-config-args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec
🟢 system test
Test
🟢Icon-install_2_6_6_gcc
🟢daint_cpu_cce
🟢daint_cpu_gcc
🟢daint_cpu_nvhpc
🟢daint_cpu_nvhpc_out_of_source
🟢daint_gpu_nvhpc
🟢daint_dsl_nvhpc

@jonasjucker
Copy link
Contributor Author

launch jenkins icon balfrin

@jenkins-apn
Copy link

tsa

🟢 unit test
Test
🟢summary

@jenkins-apn
Copy link

daint

🟢 unit test
Test
🟢summary

@jenkins-apn
Copy link

balfrin

🟢 unit test
Test
🟢summary
🟢 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🟢icon_extra-config-args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec
🟢 system test
Test
🟢Icon-install_2_6_6_gcc
🟢Icon-install_2_6_6_nvhpc
🟢Icon-install_nwp_gpu

@jonasjucker
Copy link
Contributor Author

@dominichofer Now the PR is ready for another review.

@dominichofer
Copy link
Contributor

launch jenkins icon tsa

@jenkins-apn
Copy link

daint

🟢 unit test
Test
🟢summary

@jenkins-apn
Copy link

balfrin

🟢 unit test
Test
🟢summary

@jenkins-apn
Copy link

tsa

🟢 unit test
Test
🟢summary
🟢 integration test
Test
🟢icon-spack_info
🟢icon-spack_spec
🟢icon_extra-config-args=--disable-new_feature,--enable-old_config_arg-spack_spec
🟢dace_icon.-O1-spack_spec
🟢icon_serialization=create_claw=std-spack_spec

dominichofer
dominichofer approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@dominichofer dominichofer left a comment

Choose a reason for hiding this comment

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

LGTM

@jonasjucker jonasjucker merged commit a9b5364 into main Jan 4, 2024
@jonasjucker jonasjucker deleted the icon-extra-config-arg branch January 4, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominichofer dominichofer dominichofer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ICON] Add variant for extra configure args
3 participants
@jonasjucker @jenkins-apn @dominichofer