Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Differences between Windows exe (ok) and linux gem (ko) on 5.3.2 #465

Closed
rafdulaf opened this issue Oct 8, 2013 · 23 comments
Closed

Differences between Windows exe (ok) and linux gem (ko) on 5.3.2 #465

rafdulaf opened this issue Oct 8, 2013 · 23 comments

Comments

@rafdulaf
Copy link

rafdulaf commented Oct 8, 2013

I have an issue very strange, on windows I have no warnings when building, but under linux I have strange warnings.

See that class here http://releases.ametys.org/api/3.7_LTE-dev/js/#!/api/Ametys.form.AbstractFieldsWrapper-method-setValue

  1. The method overrides itself... very strange :) - same on windows and linux
  2. when building this doc I have no warning under windows and I have the following warning under linux : AbstractFieldsWrapper.js:187: @fires references unknown event: autosize

In my code, the event 'autosize' only exists on the component boxselect http://releases.ametys.org/api/3.7_LTE-dev/js/#!/api/Ext.ux.form.field.BoxSelect that is currently miss-writen for the doc (and excluded from my warnings)
This event also exists in extjs TextField and TextareaField

My questions are

  1. why do I have warnings under linux and not under windows ?
  2. do you think this wrong warning is due to the bad documented class BoxSelect ?
@nene
Copy link
Member

nene commented Oct 8, 2013

This seems to be the same strange problem (#445) that I discovered some time ago in ExtJS. I wasn't able to pin it down back then, and now it has disappeared from this place in ExtJS.

It seems to be the kind of problem that crops up when things happen in certain order. Like the files are probably loaded in different order on Win v/s Linux.

You can try turning off multiprocessing to see if this makes difference --processes=0.

You can also try limiting the scope of the problem. Like have JSDuck only parse AbstractFieldsWrapper.js and as little other files as possible.

PS. Is the bug stable? Do you get it consistently on every run?

@rafdulaf
Copy link
Author

rafdulaf commented Oct 8, 2013

yes the bug is stable
I try with --processes=0

@rafdulaf
Copy link
Author

rafdulaf commented Oct 8, 2013

No differences with --processes=0

I am doing some tests about the "fires"

At the begining I had the warning 4 times : 1 on AbstractFieldsWrapper.js and 3 on GeoCode.js

  • If I build the doc with a reduced number of files (excluding GeoCode) I have no more errors.
  • If I build the doc with a larger number of files (excluding GeoCode) I have 1 error on AbstractFieldsWrapper.js.

I am doing some tests about the "override itself" bug

Building AbstractFieldsWrapper.js with its parent AbstractField.js and Extjs is fine. And after a few test, building all my doc without a single file is fine... so I think this file (that inherit from AbstractFieldsWrapper.js) is reponsible for this bug.

So I built AbstractField.js, AbstractFieldsWrapper.js and ChangePassword.js and the bug is back. ChangePassword is extending AbstractFieldsWrapper.js and redefining the setValue method that has the bug.

For the moment I had no doc at all on ChangePassword#setValue. If I add @inheritdoc on it, everything is fine.

Question 1: is @inheritdoc mandatory ? or is this a bug ?

Question 2: the generated doc with @inheritdoc alone is different from nodoc at all (since the doc silently ignore the override)... is this a feature ? from my point of view, I would like that no doc, is the same as /** @inheritdoc */ OR a warning that also warn about nodoc even when overriding

@rafdulaf
Copy link
Author

rafdulaf commented Oct 9, 2013

I open another ticket to talk about the "override itself" issue (#468)
I open another ticket to talk about the differences between no doc and @inheritdoc (#469)

So we can now focus on the initial issue here, why do I have ununderstandable warning on linux and its ok on windows.
I'am doing some other tests and I will keep you in touch

@nene
Copy link
Member

nene commented Oct 9, 2013

I'm pretty sure it's all part of the same issue. JSDuck mistakenly detects that a method is overriding itself, and continuing from that broken state, all kinds of other problems can pop up. So I'm closing down the other issues.

Regarding @inheritdoc. When parent class documents a method and a child class includes the same method without any documentation, JSDuck treats it as implicit @inheritdoc. However if the child class method has a doc-comment (even an empty one) one would have to add explicit @inheritdoc, otherwise no docs from parent would get inherited.

The detection of overrides is somewhat related to inheritdoc implementation - they work in sequence, affecting each other a bit. So these are definitely related, plus the whole inheritdoc logic is somewhat convulated, being a good growing field for bugs.

The implementation of @fires looks up events from a class and all of its parents, making the use of the same logic that's responsible for generating the overrides information. So there is a relation here.

I tried building docs with the three files of yours: AbstractField.js, AbstractFieldsWrapper.js, ChangePassword.js. But I'm unable to reproduce any of the problems that you describe.

You haven't mentioned which version of Ruby are you running. This could help. If you can run different Ruby versions by yourself, you could check if the problem comes up in all of them.

@rafdulaf
Copy link
Author

rafdulaf commented Oct 9, 2013

The bug of detection of override is the same under linux and windows :
for the ruby version, under windows, it is the embeded one in the exe, and under linux it seems that we have two versions installed 1.9.3 et 1.8.7... but we are not sure which one is used... any idea ?

To reproduce, you can get the 3 files here http://people.anyware-services.com/~raphael/jsduck/
See the screenshot for the doc built with extjs 4.2.1
capture
Our built options are:
jsduck pathtoourjs pathtoext/src pathtoext/examples/us --builtin-classes --warnings=+nodoc(param,private),+nodoc(member,private),+nodoc(class,private),+param_count,+fires,-all:${ext.directory},-all:BoxSelect.js --images pathtoext/docs/images --template ourstrictcopyofdefaulttemplate --tags ourtags.rb --cache --cache-dir acachedir --output docs --ignore-global --title=Ametys 3.7 JS API -- footer='js doc' --external=tinymce.Editor --external=google.maps.Map --external=google.maps.LatLng

Note that adding manually /*@inheritdoc/ removes this problem

By the way, currently, when I override a method with no doc, and with a single /** @inheritdoc */ : the doc is not the same on this method... I didn't get if you say that it is a bug or you don't think its true?

@nene
Copy link
Member

nene commented Oct 9, 2013

I think you can get the Ruby version by doing:

$ cat $(which jsduck)

This should show the RubyGems generated file and the first line should tell you the version of Ruby.

@nene
Copy link
Member

nene commented Oct 9, 2013

Success. By including Ext JS sources in addition to these three files, I managed to reproduce the overrides bug.

I've also managed to reduce it further down to just ext-4.2.1/src/form/field/Field.js + those three files.

I'll try to digg further.

@nene
Copy link
Member

nene commented Oct 9, 2013

OK. I think I've got this problem down now. I have reduced it to:

/** */
Ext.define('Base', {
    /** */
    foo: 1
});

/** */
Ext.define('Child', {
    extend:'Base',

    foo: 2
});

/** */
Ext.define('GrandChild', {
    extend: 'Child',

    foo: 3
});

/** */
Ext.define('GrandGrandChild', {
    extend: 'GrandChild',

    foo: 4
});

Which results in Child, GrandChild and GrandGrandChild all having a property foo, that declares:

Overrides: Base.foo, Child.foo, GrandChild.foo

So, it looks like they all end up referencing the same object that lists the overridden items - so a copy operation is missing from somewhere.

nene added a commit that referenced this issue Oct 9, 2013
Overrides shouldn't be inherited, but the code in InheritMembers
happened to inherit them along with all the other things, causing
auto-detected members to share the same overrides array, resulting
in bogus lists of overridden members.

This fix should take care of bug #465.
@nene
Copy link
Member

nene commented Oct 9, 2013

I think I got this down now.

Pushed out jsduck-5.3.3 (gem release only) for you to try out and see if this fixed this problem and possibly others.

@rafdulaf
Copy link
Author

rafdulaf commented Oct 9, 2013

We are using ruby-1.9.3

With jsduck-5.3.3 the override itself problem is solved.
The difference between nodoc and /** @inheritdoc */ has also gone.
But the @fires autosize warning is still here. I'm working on giving you a small test to reproduce it.

@rafdulaf
Copy link
Author

Busted!

First, thank you for the above fix.
Now I have found how to reproduce the bug of @fire. For recall, its an abusive warning that I only have on linux. Moreover the generated doc is different under windows and linux, ok on windows, but under linux the list of fired events is wrong.

How to reproduce ? you will find here http://people.anyware-services.com/~raphael/jsduck/ 3 classes.
AbstractFieldWrapper inherit from AbstractField that extend FieldContainer and mixin Field, and another class ScriptEditor that extend Textarea... I don't understand why this class does interact... :)

@nene
Copy link
Member

nene commented Oct 11, 2013

Unable to reproduce.

I included these three classes, but got no warnings about @fires. Running it like so:

$ jsduck -o docs/ raphael/AbstractField.js raphael/AbstractFieldsWrapper.js raphael/ScriptEditor.js --warnings=-all,+fires

Moreover the generated doc is different under windows and linux, ok on windows, but under linux the list of fired events is wrong.

Could you be more exact. What's exactly the difference?

@rafdulaf
Copy link
Author

We did big changes on this class yesterday, the bug is still there but now on checkChange (instead of setValue)

Under windows it says it fires : change, dirtychange and validitychange
Under linux it says it fires : change, autosize (while the textual doc says it will fire change, dirtychange and validitychange)

Here is our command line
jsduck pathtoourjs pathtoext/src pathtoext/examples/us --builtin-classes --warnings=+nodoc(param,private),+nodoc(member,private),+nodoc(class,private),+param_count,+fires,-all:${ext.directory},-all:BoxSelect.js --images pathtoext/docs/images --template ourstrictcopyofdefaulttemplate --tags ourtags.rb --cache --cache-dir acachedir --output docs --ignore-global --title=Ametys 3.7 JS API -- footer='js doc' --external=tinymce.Editor --external=google.maps.Map --external=google.maps.LatLng

@rafdulaf
Copy link
Author

So, we did an ultimate code restructuration and the bug has now disapear. I propose to close the ticket for the moment. I will open another if it happens again in the future.

@nene
Copy link
Member

nene commented Oct 11, 2013

OK then.

I only see change, dirtychange and validtychange events listed for AbstractFieldsWrapper#setValue.

@nene nene closed this as completed Oct 11, 2013
@rafdulaf
Copy link
Author

Yes as I do under windows. Thank you.

@ryan-nauman
Copy link

@nene Will you be publishing a windows binary of 5.3.3?

@nene
Copy link
Member

nene commented Oct 15, 2013

I've released 5.3.4 instead.

@ryan-nauman
Copy link

Great, thanks. I'll keep an eye on sourceforge for it.

@nene
Copy link
Member

nene commented Oct 15, 2013

Check the releases page instead: https://github.com/senchalabs/jsduck/releases

All windows binary are now added in there.

@ryan-nauman
Copy link

Oh, awesome! I didn't even notice because 5.3.3 didn't have one. You might want to consider adding a note on the sourceforge page of this unless you will run them in parallel.

@nene
Copy link
Member

nene commented Oct 15, 2013

I've added a note to the Sourceforge page, and will be removing the downloads from there some time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants