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

Object-centric MetaLink integration. #2120

Merged
merged 8 commits into from Jan 29, 2019

Conversation

Projects
None yet
8 participants
@StevenCostiou
Copy link
Contributor

StevenCostiou commented Dec 21, 2018

  • Object-centric MetaLink
  • Permalinks
  • Object and Class API to install links (objects and classes)

Tests also added. Everything should be separated from the core, so features should be within their own private bubble.

]

{ #category : #'*Reflectivity' }
Object >> link: aMetaLink toAST: aNode [

This comment has been minimized.

@dionisiydk

dionisiydk Dec 21, 2018

Contributor

Can you comment why methods on Object are required?

This comment has been minimized.

@dionisiydk

dionisiydk Dec 21, 2018

Contributor

In most cases you delegate to target objects which is good. But why this Object methods are good idea?

This comment has been minimized.

@StevenCostiou

StevenCostiou Dec 21, 2018

Author Contributor

The idea is to have an interface directly from objects to scope metalink to them instead of calling complex metalink API.

For instance: anObject link: metalink toAST: aNode
instead of: aNode link: metalink forObject: anObject

Same for classes.

The link:toAST: is the most generic interface that does not make assumptions on the kind of node it must install the link.

This comment has been minimized.

@dionisiydk

dionisiydk Dec 21, 2018

Contributor

I see. I misunderstood it originally.
So anObject here is where metalink will be active. For other receivers metalink will be not executed for given ast-node. Right?

This comment has been minimized.

@StevenCostiou

StevenCostiou Dec 22, 2018

Author Contributor

Yes :)

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Dec 27, 2018

Steven can you have a look because some failing tests are
osx-32 / Tests-osx-32 / testCallToSuper – MacOSX32.Reflectivity.Tests.LinkInstallerTests

@StevenCostiou

This comment has been minimized.

Copy link
Contributor Author

StevenCostiou commented Dec 27, 2018

Steven can you have a look because some failing tests are
osx-32 / Tests-osx-32 / testCallToSuper – MacOSX32.Reflectivity.Tests.LinkInstallerTests

Where can i see the failing test, because in jenkins i do not see it and the test is green on my machine :'(

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Dec 27, 2018

Click on Details (above) then on the Test in the "menu bar"

@StevenCostiou

This comment has been minimized.

Copy link
Contributor Author

StevenCostiou commented Dec 27, 2018

Thanks. Ok this is because of remaining metalinks on breakpoint test classes which are created and removed (by the test) from the system. Somehow metalinks are not uninstalled from those classes. See BreakpointTest>>newDummyClass. I will look at it.

@jecisc

This comment has been minimized.

Copy link
Member

jecisc commented Jan 3, 2019

I have the impression it's too late for Pharo 7. It should probably be rebased in Pharo 8.

@StevenCostiou

This comment has been minimized.

Copy link
Contributor Author

StevenCostiou commented Jan 3, 2019

OK. How do i do that? Note that the problem is not in this code, but is due to a metalink bug already in Pharo 7 (I need to report that bug...).

@astares

This comment has been minimized.

Copy link
Member

astares commented Jan 3, 2019

There is now a "Pharo7.0" branch and additionally also a "Pharo8.0" branch. You need to do a PR agains the P8 one.

@estebanlm

This comment has been minimized.

Copy link
Member

estebanlm commented Jan 3, 2019

@StevenCostiou

This comment has been minimized.

Copy link
Contributor Author

StevenCostiou commented Jan 3, 2019

Should i do that now or can it wait next week? I would like to check with Marcus, but if this is urgent i can edit that today.

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Jan 3, 2019

I do not see if this is just a better API to reflectivity why this cannot be added to P7?

@StevenCostiou

This comment has been minimized.

Copy link
Contributor Author

StevenCostiou commented Jan 3, 2019

Well all the PR code is in the "Reflectivity bubble", and should not affect the rest of the system. But three tests are failing because of a bug triggered by breakpoint tests executed before, for which i can provide a temporary fix in my code but i need to talk to Marcus for fixing the real bug - so it would have to wait next week... I will submit a bug today and a mailing list question. Thing is i understood it seemed to be rather urgent...

StevenCostiou added some commits Jan 8, 2019

Temporary workaround: metalinks stay installed on method even when th…
…ey were removed. In that case it tries to find a method which does not exists when uninstalling the link. The impact of this change is limited to Reflectivity, specifically the uninstallation of remaining links on obsolete classes.
Workaround improved to forbid trying to uninstall a link from a metho…
…d that does not exist in the class on which it is supposed to be (class made obsolete with niled method dictionary)
@StevenCostiou

This comment has been minimized.

Copy link
Contributor Author

StevenCostiou commented Jan 8, 2019

There are no more errors in tests related to Reflectivity.

@guillep

This comment has been minimized.

Copy link
Member

guillep commented Jan 9, 2019

Still it's strange that there are 12 broken tests, when the pharo build is mostly green.

@guillep

This comment has been minimized.

Copy link
Member

guillep commented Jan 9, 2019

Also, if this is for Pharo7 you should open an issue in fogbugz related to it

@tesonep

This comment has been minimized.

Copy link
Collaborator

tesonep commented Jan 9, 2019

Hi @StevenCostiou does this have a related issue in fogbugz. How we can tested if it is working?
Are there new packages? or only updated old ones?

@StevenCostiou

This comment has been minimized.

Copy link
Contributor Author

StevenCostiou commented Jan 9, 2019

Yes, this is fogbugz 22830. There are no new packages, only update of the Reflectivity packages. The code can be loaded from this branch https://github.com/StevenCostiou/pharo/tree/22830-Object-centric-metalink-integration but tests are passing. I will try again a full Pharo test on my image.

@estebanlm estebanlm added this to the 8.0.0 milestone Jan 11, 2019

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Jan 29, 2019

Failing tests are unrelated!

@Ducasse Ducasse merged commit 8005e22 into pharo-project:Pharo7.0 Jan 29, 2019

1 check failed

continuous-integration/jenkins/pr-merge This commit has test failures
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.