-
Notifications
You must be signed in to change notification settings - Fork 22
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
Update GraalJS dependency #111
Conversation
We now use JSDynamicObject instead of DynamicObject in most places.
@BarrensZeppelin Many thanks for the contribution. It is great that we can get some help from the open-source community to help maintain NodeProf and keep it up-to-date. I have created a new branch (graalvm-22.1.0) based on what it is now on master and will do a new release based on that. |
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.
Overall looks good, left a few comments.
src/ch.usi.inf.nodeprof/src/ch/usi/inf/nodeprof/jalangi/factory/AbstractFactory.java
Outdated
Show resolved
Hide resolved
4e1e395
to
233bf98
Compare
Thanks for the feedback. 🙂 |
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.
LGTM, many thanks for the contribution.
@BarrensZeppelin can you try |
The issues should be fixed now 🤞 I tried to set up checkstyle in my IDE through |
Hmm, I'll have a look at the NPM test failure locally... EDIT: Maybe |
Since GraalJS 22.1, getNodeObject() for a JSBinaryNode can return null if the node does not have a NodeInfo annotation, which the InNode doesn't have.
Let's do that in a follow up. |
Well, now I'm at a loss. |
@BarrensZeppelin How about skipping that unit test first, we can track that in a separate issue and follow up on that. Keeping the master up-to-date with the latest graal.js is more important in my opinion. |
Alright, I've added a temporary work-around for now. |
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.
@BarrensZeppelin Now that all tests are passing, let's merge the changes. Thanks for the fix!
I'm attempting to make nodeprof work on newer versions of GraalJS.
Currently I've updated GraalJS to version 23.3.3.
The changes are relatively simple, the tests pass, and I've run some of our dynamic analyses using the updated version without issues, so I think the risk of breaking everything is quite small.
Still, it would be nice if more users could test it out.
I don't think there's an official communication channel for nodeprof, so I'm just tagging some people that have opened issues recently. If you have the time to test the changes, it would very helpful. 🙂
@mwaldrich @MadhuNimmo @Soulike @alexjordan