Skip to content

Remove support for jextract -interactive - #18230

Merged
keithc-ca merged 1 commit into
eclipse-openj9:masterfrom
TheMarvelFan:master
Oct 10, 2023
Merged

Remove support for jextract -interactive#18230
keithc-ca merged 1 commit into
eclipse-openj9:masterfrom
TheMarvelFan:master

Conversation

@TheMarvelFan

Copy link
Copy Markdown
Contributor

Referencing issue #10764
I am not sure how to test this change on my local environment, but I am sure that its support has been ended. Please do point out any error or some additional change that needs to be made and I might have overlooked, and feel free to leave reviews about this contribution. PS: I am new to open source contribution and this will be my first actual pull request, so please provide guidance if I have made any errors.

@keithc-ca keithc-ca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove the (now) unused code:

  • Main.runInteractive()
  • supporting imports
  • Main.doCommand() and associated native code, exports, etc. (jextraceglue.c, jextractnatives_internal.h, jextractnatives.c, jextractnatives_api.h, module.xml, CMakeLists.txt)

Comment thread jcl/src/openj9.dtfj/share/classes/com/ibm/jvm/j9/dump/extract/Main.java Outdated
@keithc-ca keithc-ca self-assigned this Oct 4, 2023
@keithc-ca

Copy link
Copy Markdown
Contributor

how to test this change

See https://github.com/eclipse-openj9/openj9/blob/master/doc/build-instructions/Build_Instructions_V21.md for how to set up your system and build (Java 21 in this case) locally.

@TheMarvelFan

Copy link
Copy Markdown
Contributor Author

@keithc-ca Thanks for pointing these errors out. I am working on the exports, and have made the changes you mentioned in the Java source code.

Comment thread jcl/src/openj9.dtfj/share/classes/com/ibm/jvm/j9/dump/extract/Main.java Outdated
Comment thread jcl/src/openj9.dtfj/share/classes/com/ibm/jvm/j9/dump/extract/Main.java Outdated
Comment thread jcl/src/openj9.dtfj/share/classes/com/ibm/jvm/j9/dump/extract/Main.java Outdated
Comment thread jcl/src/openj9.dtfj/share/classes/com/ibm/jvm/j9/dump/extract/Main.java Outdated
Comment thread runtime/oti/jextractnatives_api.h Outdated
Comment thread runtime/oti/jextractnatives_api.h Outdated
@TheMarvelFan

Copy link
Copy Markdown
Contributor Author

@keithc-ca Thanks for the reviews. I hope everything is in order now.

Comment thread runtime/jextractnatives/CMakeLists.txt Outdated
Comment thread runtime/jextractnatives/jextractnatives.c Outdated
@keithc-ca

Copy link
Copy Markdown
Contributor

The overall change looks good. I think it's time to rebase and squash to a single commit.

@TheMarvelFan

Copy link
Copy Markdown
Contributor Author

@keithc-ca Hi Keith. I have squashed all the commits into one. Thanks a lot for your cooperation and patience, and apologies for any trouble I may have caused. I learnt a lot of new things while I contributed to this issue, and I will put my knowledge to good use.

@keithc-ca

Copy link
Copy Markdown
Contributor

Thank you for squashing to a single commit. It would be nice if you could rebase on a more recent commit on the master branch, but that's less important.

I would like you to improve the commit message; see [1] for guidance. The first line should summarize what merging the commit will do (the summary here, "Remove support for jextract -interactive", would be fine). Normally, the body would provide more detail, but I'm not sure there's much more to say. On the other hand, I don't think it's helpful to have text like "removed blank line" or "removed another extra line", etc.

[1] https://github.com/eclipse-openj9/openj9/blob/master/CONTRIBUTING.md

@TheMarvelFan
TheMarvelFan force-pushed the master branch 2 times, most recently from 4317b15 to bb5ffa4 Compare October 10, 2023 16:48
@keithc-ca

Copy link
Copy Markdown
Contributor

First, a minor nit: the first line of the commit message should start with "Remove", not "Removed".

I'm not sure where the other 19 commits came from, but I think what you want to do is

git fetch --all
git rebase -i 5570f22bf9da

This will open an editor showing

pick 29d12dea56a Add tests for Value Type System.arraycopy transformation
pick bb5ffa41a84 Removed support for jextract -interactive

# other text with instructions...

You want to change "pick" to "drop" on the first line. Save the file and exit the editor. Git will skip commit 29d12dea56a and create a new commit with the other changes.

You'll want to remove what I expect is some intermediate temporary file

git rm runtime/compiler/build/scripts/RCa05068

and then amend that commit

git commit --amend

Again, git will open an editor allowing you to update the (amended) commit message; here's your change to remove that unwanted "d".

This commit will remove support for '-interactive' flag from jextract executable.
  1. Removed the method responsible for dealing with '-interactive' flag
  2. Removed unused imports required by said method
  3. Removed unused exports of said method in other files
  4. Removed the following files which are now unused:
		runtime/jextractnatives/jextractnatives_internal.h
		runtime/jextractnatives/jextractglue.c
  5. Reordered imports and removed empty lines
@TheMarvelFan

Copy link
Copy Markdown
Contributor Author

@keithc-ca Apologies for those extra commits being added. I pushed to origin right after fetching master by mistake, and that might be what caused those extra commits to show. Thanks for the help.

@keithc-ca

Copy link
Copy Markdown
Contributor

Jenkins compile aix,alinux,win jdk17

@keithc-ca

Copy link
Copy Markdown
Contributor

Jenkins test sanity amac jdk11

@keithc-ca
keithc-ca merged commit 0d83d4d into eclipse-openj9:master Oct 10, 2023
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.

2 participants