1032275 - switch cfx to ignore missing modules by default

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Tomislav Jovanovic :zombie]

if we are releasing 1.17 solely because of new modules, we better make sure we don't need to do it again.

(and there are other, less important, convenience reasons)

Comment 1

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1530

i opted to print a warning message, to be able to catch other, unintended missing modules..

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8448179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1530

This doesn't really help us unless it's the default. Why wouldn't this just be the default?

Comment 3

[Tomislav Jovanovic :zombie]

if this flag was present in 1.16, we wouldn't need to release 1.17, we could just document it and point people to use it on any MDN page that references modules not present in 1.16.

using the same logic, this could help even if it's not the default, imho.

i'm not that familiar with all the consequences of making this the default behavior, if/how it could impact AMO reviews, etc, so i didn't want to make it the default.

but if you think we can, i can do it.


(and as i said, even if we decide not to release 1.17, this flag is useful in a few other cases, like using require() from devtools loader's, or for JSMs, which all need hacks in code now, and will not need them once we switch to native and/or jpm).

Comment 4

[Dave Townsend [:mossop]]

(In reply to Tomislav Jovanovic [:zombie] from comment #3)
> if this flag was present in 1.16, we wouldn't need to release 1.17, we could
> just document it and point people to use it on any MDN page that references
> modules not present in 1.16.
> 
> using the same logic, this could help even if it's not the default, imho.
> 
> i'm not that familiar with all the consequences of making this the default
> behavior, if/how it could impact AMO reviews, etc, so i didn't want to make
> it the default.
> 
> but if you think we can, i can do it.

I don't know if we can, we should find out because making it the default is definitely preferable.

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8448179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1530

after some more thought, i think we should definitively switch to ignoring missing modules by default.

doing this in cfx, with a warning is a good half-step toward 1) jpm and 2) native jetpack, which 1) will not stop on missing modules, or 2) don't even have a packing step where they could stop.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8448179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1530

I'd rather Irakli looks at this if he has the time

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8448179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1530

patch looks ok although I'm not fully aware of all the cfx code paths to say if there is one that won't be covered by this change. I reject this review as I expect at least one test with a package that has a missing dependency. Other than that everything looks fine to me.

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8448179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1530

> patch looks ok although I'm not fully aware of all the cfx code paths to say
> if there is one that won't be covered by this change. 

manifest.py is the only place where ModuleNotFoundError was raised, so i'm pretty sure all the module-finding logic is confined to that process_module() method.


> I reject this review as I expect at least one test with a package that has
> a missing dependency.

i already added that, i used the test that previously asserted the package with dependencies missing was throwing ModuleNotFoundError, and now doesn't, but trows again if passed abort_on_missing flag:

https://github.com/zombie/addon-sdk/commit/95a666fd2a493cf317f6759c42893f9eff721330#diff-fa4ddd043fa15b684054a5dc617cd495R62

so i just made that test more specific and explicit. 

testing this with an actual addon that runs in firefox would probably be difficult, as it would need a module that exists in firefox but doesn't exist in addon-sdk/lib/sdk!

(but naturally, i tested our test addons manually after removing modules from lib/sdk)

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8448179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1530

Looks good to me.

Comment 10

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0c597e6cf848ab37605039372b27782320e503d9
bug 1032275 - flag name, more explicit test

https://github.com/mozilla/addon-sdk/commit/c5177e0685c1a7fa9d05000d146a55be996f8d08
Merge pull request #1530 from zombie/1032275-ignore-missing

bug 1032275 - cfx to ignore missing modules by default, r=@gozala

Comment 11

[Dave Townsend [:mossop]]

Can you please cherry-pick this to the other branches (firefox32, firefox31 and firefox30)?

Comment 12

[Tomislav Jovanovic :zombie]

i messed up the bug number in the original PR, so that PR consists of two commits.

i squashed and used the right commit message for this cherry-picking.

also, i only pushed to firefox32 and firefox31 branches, given that firefox 30 was released a month ago and is on its way to being history in a week or so..