easybuild-framework icon indicating copy to clipboard operation
easybuild-framework copied to clipboard

Deprecate failure to resolve a template value

Open Flamefire opened this issue 5 years ago • 12 comments

A failure in template resolving is currently hidden in the log although it is likely a bug in the code (either EasyBuild, EasyBlock or EasyConfig) This may lead to cryptic error messages later on or misbehave silently which is even worse. To not break builds completely deprecate continuing for now and promote to a hard error later. The message is now highly visible and existing cases can be fixed until the next release.

I did an extensive run during development of https://github.com/easybuilders/easybuild-framework/pull/3129 with this issue promoted to a hard failure and did not encounter a single instance. So I think there should be no issues with existing ECs (or very few)

@boegel To have enough time to check for existing issues until next release I recommend merging this soon.

Flamefire avatar Apr 17 '20 12:04 Flamefire

Fleshed out some changes into #3287, there are still some logic issues that would need resolving:

  • the cfg['ext_list']['options'] should not be resolved by cfg as this would be using the wrong values
  • More general: fetch_extension_sources should run after creating the instances to avoid duplicating the logic of updating the template dict values which has the potential of becoming out of sync
  • One failure is with the python multidep as for some reason the dependencies are flattened but resolved but resolval is partial as pyver isn't set yet. This mirrors in short/long_mod_name. Not sure if this is a problem but it may.
  • final failure is due to test yep files differing from the ec files (yep files use some replacement in yep, while ec use %(name)s replacements). IMO this is a mistake and the files should be equivalent (before templating in EB)

Flamefire avatar Apr 17 '20 14:04 Flamefire

@Flamefire conflict resolution and merge with develop please.

akesandgren avatar May 02 '20 12:05 akesandgren

@akesandgren Done, so this is ready

Flamefire avatar Jul 26 '22 12:07 Flamefire

This one's too complicated for me, someone else that feels confident around that code needs to do the review.

akesandgren avatar Jul 26 '22 16:07 akesandgren

@Flamefire Can you do a sync with develop here too, so the correct set of configurations for the test suite runs is used in CI?

boegel avatar Aug 03 '22 13:08 boegel

Rebased.

Flamefire avatar Aug 04 '22 07:08 Flamefire

@Flamefire Can this be re-targeted to the 5.0.x branch?

boegel avatar Nov 13 '23 08:11 boegel

@Flamefire Can this be re-targeted to the 5.0.x branch?

Sure can do. Do we want the deprecation there or the failure? For me the deprecation would be good to have "yesterday" and failure in 5.x

Flamefire avatar Nov 13 '23 09:11 Flamefire

@Flamefire Can this be re-targeted to the 5.0.x branch?

Sure can do. Do we want the deprecation there or the failure? For me the deprecation would be good to have "yesterday" and failure in 5.x

That works for me, but only if we can ensure that not everyone who uses EasyBuild 4.9.0 will be exposed to deprecation warnings because we didn't do the necessary work to try and avoid them.

For EasyBuild 5.0 we have an opportunity to make it a hard failure, so let's try doing that. Worst case we make it just a warning again later if it turns out that was a bit ambitious...

boegel avatar Dec 06 '23 12:12 boegel

That works for me, but only if we can ensure that not everyone who uses EasyBuild 4.9.0 will be exposed to deprecation warnings because we didn't do the necessary work to try and avoid them.

I already did some work for that all over the place in this PR.
Yes there might be some fallout but the only ways to find those is to do extensive test reinstalls with this turned to an error or just let it be as-is: A deprecation that this will be an error in 5.0 (as you seem to agree) and users can ignore this or better: report this. So I don't see it as necessarily bad if it bothers someone ;-)

As mentioned it would have been good to have the deprecation in yesterday such that we could have had time to find common remaining issues.

For EasyBuild 5.0 we have an opportunity to make it a hard failure, so let's try doing that. Worst case we make it just a warning again later if it turns out that was a bit ambitious...

Might even need to deprecate it targetting 5.1 instead of 5.0 and/or allow this to pass via a switch. IIRC we have an option to allow e.g. deprecated toolchains which by default would error, don't we? We could use the same here for a version or 2. But the earlier we get this tested a lot the better.

Flamefire avatar Dec 06 '23 19:12 Flamefire

I found the option: --silence-deprecation-warnings=resolve-templates will now silence the warning and even avoid the failure if this gets merged into 5.0 as-is. What do you say @boegel , is this enough now?

Flamefire avatar Dec 07 '23 13:12 Flamefire

PR for 5.x (with hard error): https://github.com/easybuilders/easybuild-framework/pull/4516

Flamefire avatar Apr 25 '24 13:04 Flamefire