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

Eta573 #721

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Eta573 #721

wants to merge 2 commits into from

Conversation

rajcspsg
Copy link

@rajcspsg rajcspsg commented Apr 2, 2018

@rahulmutt - I've updated the changes for the issue eta 573. Looks like I've created PR against wrong branch. could you please let me know the correct branch. I will create different PR against correct branch.

@CLAassistant
Copy link

CLAassistant commented Apr 2, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Rajkumar Natarajan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rahulmutt
Copy link
Member

Hi @rajcspsg, it looks like you need to update your fork's master to the current master for eta and then make you changes on top of a new branch that starts from the updated master in yor fork.

@rajcspsg
Copy link
Author

rajcspsg commented Apr 2, 2018

Thanks @rahulmutt. I've updated the PR.
Mostly I've updated the class package and refactored the usages of those classes in this PR. Only in this class https://github.com/typelead/eta/pull/721/files#diff-2f5c68896b14fe36169b53a471cd71b9 I've added getter/setters, since the default access modifiers for those fields doesn't let me to use the same in different package.

@rahulmutt
Copy link
Member

Looks like the build failed: https://circleci.com/gh/typelead/eta/1467?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Can you make sure you run ./cleaninstall.sh with your changes and made it pass locally?

thunkExt
| isTopLevel lfTopLevelFlag = mempty
| hasStdLayout = T.pack (show fvs)
| hasStdLayout = T.pack ((++) (show fvs) "runtime/thunk/")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rahulmutt I've tried | hasStdLayout = T.pack ((++) (show fvs) "runtime/thunk/") and | hasStdLayout = T.pack ((++) "runtime/thunk/" (show fvs) ) still I get Caused by: java.lang.ClassNotFoundException: eta.UpdatableThunk5runtime.thunk.error. Any idea?

Copy link
Member

@rahulmutt rahulmutt Apr 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you trying to add runtime/thunk/ instead of removing it above? Have you changed the names of all the eta/runtime/thunk/*Thunk* classes to eta/*Thunk*?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rahulmutt The classes Thunk, UpdatableThunk are only moved to package eta.
The remaining Thunk classes are present in package package eta/runtime/thunk.
when I make package as eta , I get getting CNFE for java.lang.NoClassDefFoundError: eta/UpdatableThunk2.
when I make package as eta/runtime/thunk/ , I get getting CNFE for java.lang.NoClassDefFoundError: eta/runtime/thunk/UpdatableThunk.
so, I'm trying to find way to include both eta/UpdatableThunk and eta/runtime/thunk/**.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be easier if you shifted all the eta/runtime/thunk/** classes to eta/*. That way you won't have to add special cases in the logic.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rahulmutt - We could do that but I feel that makes rts code bit unorganized correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to move up the specialized thunks and functions up to the eta package. They will be exposed in the public runtime API for those who need it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants