1. SCons
  2. Core
  3. SCons
  4. Pull requests

Pull requests

#133 Merged
Repository
dirkbaechle
Branch
default
Repository
scons
Branch
default

Fix: NoClean for multi-target builders

Author
  1. dirkbaechle
Reviewers
Description

This patch was prepared by Amir Szekely and fixes issue #2353. So far, the remove method would only ever check the first entry in the list of targets for a node. As also reported by Robert Zeigler in a recent mail on the user ML, users expect to be able to set the "noclean" flag for each single target. This behaviour is also in sync with our current documentation.

Comments (4)

  1. anatoly techtonik

    LGTM with minor issues.

    1. self._clean_targets(1) - I'd rewrite this to use True/False and keyword argument for readability, e.g. dryrun=False
    2. Does the test tests former behavior? I mean this is test for bug #2353
    +Command(['5.out', '6.out'], 'SConstruct.multi', action)
    +NoClean('6.out')
    ...
    +test.must_not_exist('5.out')
    +test.must_exist('6.out')
    

    And what is the test for former behavior?

    +Command(['5.out', '6.out'], 'SConstruct.multi', action)
    +NoClean('5.out')
    ...
    +test.must_not_exist('5.out')
    +test.must_not_exist('6.out')
    

    ?

    1. dirkbaechle author

      Anatoly,

      thanks for your comments and the approval. I updated the pull request by switching the argument "remove" to type bool.

      I'm not sure that I understand your comment about testing former behaviour. This a fix, which is supposed to change behaviour. So, if we would test the former behaviour, this would mean that the test would always break after the patch got applied. Instead, it's the other way round: the test breaks before applying the patch...but succeeds afterwards.

      Dirk