1219302 - Support removing files using the preprocessor when a flag is enabled

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Build, defect)

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] samgiles:support-remove-in-preprocessor > mozilla-b2g:master

Comment 2

[sgiles]

Since IFNDEF support launched, it would be nice to remove files if a flag is set too.

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8680079 [details] [review]
[gaia] samgiles:support-remove-in-preprocessor > mozilla-b2g:master

Hey sgiles, why you want to have this flag as switcher to enable / disable? IMO, you should remove or comment your remove list to achieve what you want instead of having this flag and it feels superfluous to me.

Comment 4

[sgiles]

Removing or commenting the remove list doesn't work for my use case - the decision needs to be made based on whether 'enable' is true or false.

If you have an IFNDEF block to conditionally turn a feature off, you can't currently remove files when the define is '1'. For example:

  //IFNDEF_NO_LOCK_SCREEN
  loadTheLockScreen('/from/some/file');
  //ENDIF_NO_LOCK_SCREEN

If my 'removes' list includes '/from/some/file', it will only be removed if 'NO_LOCK_SCREEN' is not '1'.  So in the case of "NO_LOCK_SCREEN=0" the preprocessed code will look like:

  loadTheLockScreen('/from/some/file');

Yet, '/from/some/file' will have been deleted.   We need some inverse now that IFNDEF is supported.

Comment 5

[sgiles]

Comment on attachment 8680079 [details] [review]
[gaia] samgiles:support-remove-in-preprocessor > mozilla-b2g:master

I've fixed the typo, sorry about that :/

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8680079 [details] [review]
[gaia] samgiles:support-remove-in-preprocessor > mozilla-b2g:master

Hi sgiles, I think the remove list would be different if enable flag is different.

source:
-------
//IFDEF_XX
load('A');
//ENDIF

//IFNDEF_XX
load('B');
//ENDIF
-------

//remove list
remove([
  ['A']  // Remove all files in the array when the flag is disabled.
]);




if XX = 0

source:
-------
load('B')
-------

[preprocessor] remove A.
Correct! According remove list, we removed A when flag is disabled!




if XX = 1

source:
-------
load('A')
-------

Here comes out a problem, we should remove B but it didn't define in remove list yet.


So, patch seems like a workaround to me and it cannot cover the case as above mentioned. My proposal is adding another remove list for IFNDEF case. If I was wrong please correct me :) thanks!

Comment 7

[sgiles]

Hmm, good point.

What do you think of this?  Aiming to make it explicit in the config:

```
"remove": {
  "ifdef":  [ /* removes list */ ]
  "ifndef": [ /* removes list */ ]
}

```
and, for backwards compatibility:

```
"remove": [ /* removes list based on current behaviour */ ]
```

We need something to cover off the IFNDEF case.

Comment 8

[sgiles]

I updated the patch to cover the logic I've outlined in my previous comment.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8680079 [details] [review]
[gaia] samgiles:support-remove-in-preprocessor > mozilla-b2g:master

Great! LGTM for this solution.

I'm sure so far we don't have too much apps using this feature. (IIRC, only system & settings) I'll migrate them to this newer version and remove backward compatibility in another follow-up bug if I'm free.

Comment 10

[GitHub Autolander Bot]

There was an error creating the taskgraph, please try again. If the issue persists please contact someone in #taskcluster.

Comment 11

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/32818

The pull request could not be applied to the integration branch. Please try again after current integration is complete. You may need to rebase your branch against the target branch.

Comment 12

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/32818

The pull request could not be applied to the integration branch. Please try again after current integration is complete. You may need to rebase your branch against the target branch.

Comment 13

[[:fabrice] Fabrice Desré]

https://github.com/mozilla-b2g/gaia/commit/b9fe675f95ddc4402878f7e492832376193ef265