Support removing files using the preprocessor when a flag is enabled

RESOLVED FIXED

Status

Firefox OS
Gaia::Build
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sgiles, Assigned: sgiles)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
Created attachment 8680079 [details] [review]
[gaia] samgiles:support-remove-in-preprocessor > mozilla-b2g:master
(Assignee)

Updated

3 years ago
Attachment #8680079 - Flags: review?(rchien)
(Assignee)

Comment 2

3 years ago
Since IFNDEF support launched, it would be nice to remove files if a flag is set too.
(Assignee)

Updated

3 years ago
Assignee: nobody → sgiles
(Assignee)

Updated

3 years ago
Blocks: 1219075
(Assignee)

Updated

3 years ago
Blocks: 1219306
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.
Attachment #8680079 - Flags: review?(rchien) → review-
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Comment 5

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

I've fixed the typo, sorry about that :/
Attachment #8680079 - Flags: review- → review?(rchien)
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!
Attachment #8680079 - Flags: review?(rchien) → review-
(Assignee)

Comment 7

3 years ago
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.
Flags: needinfo?(rchien)
(Assignee)

Comment 8

3 years ago
I updated the patch to cover the logic I've outlined in my previous comment.
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.
Flags: needinfo?(rchien)
Attachment #8680079 - Flags: review- → review+
(Assignee)

Updated

3 years ago
Keywords: autoland

Updated

3 years ago
Keywords: autoland
There was an error creating the taskgraph, please try again. If the issue persists please contact someone in #taskcluster.
(Assignee)

Updated

3 years ago
Keywords: autoland

Updated

3 years ago
Keywords: autoland
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.
(Assignee)

Updated

3 years ago
Keywords: autoland

Updated

3 years ago
Keywords: autoland
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.
https://github.com/mozilla-b2g/gaia/commit/b9fe675f95ddc4402878f7e492832376193ef265
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.