-
Notifications
You must be signed in to change notification settings - Fork 140
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
base: master
Are you sure you want to change the base?
Eta573 #721
Conversation
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. |
Hi @rajcspsg, it looks like you need to update your fork's |
Thanks @rahulmutt. I've updated the PR. |
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 |
thunkExt | ||
| isTopLevel lfTopLevelFlag = mempty | ||
| hasStdLayout = T.pack (show fvs) | ||
| hasStdLayout = T.pack ((++) (show fvs) "runtime/thunk/") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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*
?
There was a problem hiding this comment.
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/**.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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.