[bugfix] Fix deep-copy of _Undefined constant

[giordano]

The condition self.environment is _Undefined is always False, so the error
is actually never raised.

Without this change, I get a much more obscure error message:

==============================================================================
SUMMARY OF FAILURES
------------------------------------------------------------------------------
FAILURE INFO for Test 
  * Test Description: Test
  * System partition: catalina:default
  * Environment: gnu
  * Stage directory: /home/mose/repo/reframe/stage/catalina/default/gnu/Test
  * Node list: 
  * Job type: local (id=None)
  * Dependencies (conceptual): []
  * Dependencies (actual): []
  * Maintainers: []
  * Failing phase: compile_wait
  * Rerun with '-n Test -p gnu --system catalina:default -r'
  * Reason: build error: stdout: 'rfm_Test_build.out', stderr: 'rfm_Test_build.err'
--- rfm_Test_build.err (first 10 lines) ---
/home/mose/repo/reframe/stage/catalina/default/gnu/Test/rfm_Test_build.sh: line 13: syntax error near unexpected token `newline'
/home/mose/repo/reframe/stage/catalina/default/gnu/Test/rfm_Test_build.sh: line 13: `spack env activate -V -d <reframe.core.buildsystems._UndefinedType object at 0x7effb15a5c70>'
--- rfm_Test_build.err --- 
------------------------------------------------------------------------------
Log file(s) saved in: '/tmp/rfm-mtmrgbz_.log'

With this change I get

==============================================================================
SUMMARY OF FAILURES
------------------------------------------------------------------------------
FAILURE INFO for Test 
  * Test Description: Test
  * System partition: catalina:default
  * Environment: gnu
  * Stage directory: /home/mose/repo/reframe/stage/catalina/default/gnu/Test
  * Node list: 
  * Job type: local (id=None)
  * Dependencies (conceptual): []
  * Dependencies (actual): []
  * Maintainers: []
  * Failing phase: compile
  * Rerun with '-n Test -p gnu --system catalina:default -r'
  * Reason: build system error: no Spack environment is defined
------------------------------------------------------------------------------
Log file(s) saved in: '/tmp/rfm-93cirj79.log'

which I guess was the point of this check

[jenkins-cscs]

Can I test this patch?

[jjotero]

Hi @giordano. Thanks for spotting this.

I have the feeling that the line you changed was correct before. Instead, the definition of _UndefinedType should be as follows:

class _UndefinedType:
    '''Used as an initial value for undefined values instead of None.'''
    __slots__= ()

    def __deepcopy__(self, memo):
        return self

This would ensure that the _Undefined constant will always be the same instance of _UndefinedType regardless of deep copies and so on.

Could you please try this approach instead? Thanks!

[pep8speaks]

Hello @giordano, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2021-06-24 08:51:57 UTC

[giordano]

Yes, I confirm this also does the trick, thanks!

[victorusu]

@giordano, thanks. I personally like this change much more. I was about to ask you why not work on the _Undefined since it is = to _UndefinedType?

This fix implies that we had a bug because _UndefinedType is deeply copiable... 🤔

[giordano]

Also, I wanted to add a test, but I think
https://github.com/eth-cscs/reframe/blob/9dbb9899d371e22f325feb9adeac379b398511a9/unittests/test_buildsystems.py#L281-L284
is already meant to check this situation and the test was already passing, so... this test is catching some other BuildSystemError?

[jjotero]

@giordano This test is checking this exact same BuildSystemError. The difference is that the Spack(BuildSystem) instance is deep-copied when you actually run ReFrame, and such deep-copy is not done by the unit test. Hence, the deep-copy

if self.environment is _Undefined:
    ...

compares that self.environment is the same instance of _UndefinedType as _Undefined. But because the build system class has been deep-copied, they are not he same instance and this evaluates to False. This is what overriding the __deepcopy__ method of _UndefinedType fixes.

This being said, we already have a mechanism in place to deal with these types of classes (reframe/core/variables.py) and it probably makes sense to make all these BuildSystem classes use all that machinery too. I'll create an issue to discuss that internally.

Thanks again for spotting the bug!

@victorusu are you happy with the changes?