Closed Bug 1032275 Opened 6 years ago Closed 6 years ago

switch cfx to ignore missing modules by default

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Tomislav, Assigned: Tomislav)

References

Details

Attachments

(1 file)

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)
i opted to print a warning message, to be able to catch other, unintended missing modules..
Attachment #8448179 - Flags: review?(dtownsend+bugmail)
Assignee: nobody → tomica+amo
Status: NEW → ASSIGNED
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?
Attachment #8448179 - Flags: review?(dtownsend+bugmail) → review-
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).
(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.
Flags: needinfo?(rFobic)
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.
Attachment #8448179 - Flags: review- → review?(dtownsend+bugmail)
Summary: add cfx --ignore-missing-modules argument → switch cfx to ignore missing modules by default
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
Attachment #8448179 - Flags: review?(dtownsend+bugmail) → review?(rFobic)
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.
Attachment #8448179 - Flags: review?(rFobic) → review-
Flags: needinfo?(rFobic)
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)
Attachment #8448179 - Flags: review- → review?(rFobic)
Comment on attachment 8448179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1530

Looks good to me.
Attachment #8448179 - Flags: review?(rFobic) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Can you please cherry-pick this to the other branches (firefox32, firefox31 and firefox30)?
Flags: needinfo?(tomica+amo)
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..
Flags: needinfo?(tomica+amo)
You need to log in before you can comment on or make changes to this bug.