Deprecate trj9 compiler directory #1992

fjeremic · 2018-05-25T03:46:48Z

The runtime/compiler/trj9 directory currently serves no purpose as its
parent directory is completely empty. In an effort to simplify the
codebase and closely match the directory layout of OMR we fold this
trj9 directory away so that understanding how OMR and OpenJ9 fit
together with respect to the compiler component is much easier.

Fixes: #667

Signed-off-by: Filip Jeremic [email protected]

fjeremic · 2018-05-25T03:47:09Z

This PR depends on eclipse-omr/omr#2586.

fjeremic · 2018-05-25T03:56:17Z

Jenkins test sanity depends eclipse-omr/omr#2586

fjeremic · 2018-05-25T16:48:30Z

Updated all copyrights.

Jenkins test sanity depends eclipse-omr/omr#2586

fjeremic · 2018-05-25T17:30:28Z

Windows builds are failing due to an infrastructure issue. This is a known problem. FYI to committers/reviewers.

mstoodle · 2018-05-25T18:27:36Z

I suspect I may not be the only one to be confused by, e.g. this file:
runtime/compiler/z/codegen/TreeEvaluatorTable.hpp which contains the magic line:
#include "compiler/z/codegen/TreeEvaluatorTable.hpp"

And I'm betting that's supposed to actually pull in the file from OMR:
omr/compiler/z/codegen/TreeEvaluatorTable.hpp

I would have to imagine it's the IPATH that makes it work properly, presumably by never adding the runtime/ directory to IPATH. But I had to think about it a few times to imagine how it would work.

I admire the amount of work that went into this PR, and feel bad for complaining about it, but I really think this set up will confuse even somewhat advanced developers like myself. Maybe this file (and others like it) should insert omr/ (as #include "omr/...) like you did in the .mk files to make it more clear what's happening?

fjeremic · 2018-05-25T18:33:11Z

I would have to imagine it's the IPATH that makes it work properly, presumably by never adding the runtime/ directory to IPATH. But I had to think about it a few times to imagine how it would work.

That's precisely how and why it works. The runtime/ directory is not included in the compiler's IPATH.

I admire the amount of work that went into this PR, and feel bad for complaining about it, but I really think this set up will confuse even somewhat advanced developers like myself. Maybe this file (and others like it) should insert into the #include "omr/... like you did in the .mk files to make it more clear what's happening?

This is a reasonable request. I agree. I will work on this now.

fjeremic · 2018-05-25T18:50:42Z

@mstoodle fixed in b6fea32. I also took this opportunity to rebase and pick up the fix in #1982.

Jenkins test sanity depends eclipse-omr/omr#2586

mstoodle · 2018-05-25T19:32:25Z

I like the way this change also folds away a lot of the unnecessary trj9/ directories in our #includes....Much cleaner looking!

mstoodle

I only expected to see omr/ in the #includes in header files in the compiler because those are the places that need to include their direct super class's header file. I found one in a .cpp file that I did not expect, but this change just converts what was already there:
compiler/ilgen/ClassLookahead.cpp includes omr/compiler/il/OMRTreeTop_inlines.hpp but I'm pretty sure that could just be il/OMRTreeTop_inlines.hpp but that's not a big deal and can be done independently of this already massive change.

Otherwise, it looks great. Thanks for this undertaking, @fjeremic, even though I know it came from a place of pain :) .

AdamBrousseau · 2018-05-25T20:59:49Z

jenkins test sanity win jdk9,jdk10

mstoodle · 2018-05-25T21:10:41Z

I have reviewed and approved all three PRs associated with this change, but I cannot (at this time) afford to oversee the merge process.

If any other committer wants to do the merge based on my approval of the changes (once the tests complete), that's fine. Otherwise, I'm afraid I probably won't be able to properly take care of it until Monday (sorry @fjeremic).

mstoodle · 2018-05-28T14:08:01Z

@fjeremic, I have another observation about this pull request that prevents me from merging. Sorry I didn't notice it on Friday!!

Did you use git mv to move all the files out of trj9/ or did you just use the file system to do it?

It looks to me like we've lost all the history on every file within the compiler component with this change, unless I'm mistaken.....

i.e. git log <any file name in compiler directory> only shows:

commit 7a71544d5ea28ff38bca158e4bda5e4234b77469
Author: Filip Jeremic <[email protected]>
Date:   Thu May 24 15:09:45 2018 -0400

    Move all files from compiler/trj9 to compiler
    
    In addition delete the trj9 directory all together.
    
    Signed-off-by: Filip Jeremic <[email protected]>

I would prefer not to lose our 8.5 months of history :) .

fjeremic · 2018-05-28T14:25:25Z

Did you use git mv to move all the files out of trj9/ or did you just use the file system to do it?

Ouch! Good catch. That would have been very bad. I simply used mv. It looks like in GitHub it says it was a "file rename" but indeed should have been a move. Fixing this now.

AdamBrousseau · 2018-05-28T14:34:32Z

I think Git is smart enough to know you moved it. mv and rename are the same. If you do
git log --follow -- <filename> you get the history.

fjeremic · 2018-05-28T14:52:28Z

I think Git is smart enough to know you moved it. mv and rename are the same. If you do
git log --follow -- you get the history.

You're right. I did git mv and it made no difference. Reading up more on this it seems there is no clean way of keeping the history intact other than rewriting the entire thing which will never fly in a shared repository [OpenJ9] already forked by hundreds of people. @mstoodle I don't think we have an alternative here other than fixing up our gitconfig to alias log with log --follow.

I can amend the git message with a big WARNING to use git log --follow if a user wants to see even older history? This will at least give readers a path forward. What do you think?

fjeremic · 2018-05-28T15:01:35Z

Interestingly enough comparing the old "Move files from runtimes/tr.source to runtimes/compiler" commit git does seem to have kept the history of the rename. It doesn't seem to like moving files around though.

1aa5228

vs.

7a71544

fjeremic · 2018-05-28T18:11:14Z

Pushed a fix to JIT_SRCBASE when doing out of source builds with a sane default value. Also amended ac5d80c with an *** IMPORTANT *** message regarding history and git log --follow for future readers that may get stumped by this.

fjeremic · 2018-05-28T21:31:34Z

@mstoodle as suspected the Travis CI issues have been fixed via #2002. The build has now passed after rebasing on the fix. I've launched the Jenkins builds. Hopefully these pass as well. We should be good to go here now.

fjeremic · 2018-05-28T21:32:18Z

Jenkins test sanity depends eclipse-omr/omr#2586

fjeremic · 2018-05-29T02:11:58Z

Wohoo! All 20 checks successful!

fjeremic · 2018-05-30T14:30:28Z

Just to update anyone who is watching this PR. We have merged the OMR bit but are waiting for an OMR-Acceptance [1] promotion to unblock this change. I am watching it like a hawk and helping it along with all the machine/infrastructure issues.

I will do a rebase to fix the conflicts once we are ready to merge.

[1] https://ci.eclipse.org/openj9/job/Pipeline-OMR-Acceptance/

pshipton · 2018-05-30T18:58:11Z

Because of all the PR builds, and the 8 hour timeout on the OMR acceptance, the builds started this morning are unlikely to succeed.

fjeremic · 2018-05-30T19:11:04Z

Because of all the PR builds, and the 8 hour timeout on the OMR acceptance, the builds started this morning are unlikely to succeed.

Perhaps I'll kick one off very late tonight. Hopefully the farm is not as loaded then?

pshipton · 2018-05-30T20:40:53Z

Perhaps I'll kick one off very late tonight. Hopefully the farm is not as loaded then?

We expect #2020 to help somewhat in the future. Depends on the backlog left by all the PR builds started during the day. Then there are the nightly builds that start around midnight...

I think the first one from today https://ci.eclipse.org/openj9/view/Pipelines/job/Pipeline-OMR-Acceptance/491/ is going to promote, which I believe contains your change.

fjeremic · 2018-05-30T20:48:39Z

I think the first one from today ci.eclipse.org/openj9/view/Pipelines/job/Pipeline-OMR-Acceptance/491 is going to promote, which I believe contains your change.

It just promoted actually with less than an hour until Armageddon! I'm rebasing this change now.

In addition delete the trj9 directory all together. *** IMPORTANT *** If you are using `git log` to look at the history of a particular file your history will stop at this commit due to movement of all the files in the compiler directory. To get the full history use `git log --follow` instead. Signed-off-by: Filip Jeremic <[email protected]>

Now that all files have been moved from compiler/trj9 to compiler rename the include directives appropriately. Signed-off-by: Filip Jeremic <[email protected]>

Signed-off-by: Filip Jeremic <[email protected]>

When the build system processes makefile.ftl it creates a file called makefile, thus we end up having both makefile and Makefile. This is unnecessary and these files can be consolidated. Signed-off-by: Filip Jeremic <[email protected]>

Gather list of files changed in PR using: ``` git show --pretty="oneline" --name-only 9b102f5..HEAD ``` Pipe output to file and remove commit SHAs manually, then create a script to change the headers: ``` cat files-changed | while read i; do sed -i -E 's/Copyright $c$ ([0-9]+), ([0-9]+) IBM Corp/Copyright (c) \1, 2018 IBM Corp/g' $i done ``` Signed-off-by: Filip Jeremic <[email protected]>

For an include such as: ``` #include "compiler/z/codegen/TreeEvaluatorTable.hpp" ``` The intention of the include is not very clear. The above include is supposed to include the OMR version of the tree evaluator table. However this is not very clear just from the include path since the same path now exists in OpenJ9. The reason why OMR's version will be picked is because the /openj9/runtime directory is not included in the compilers IPATH directives. Unless someone knows this it would be hard to determine which include actually gets picked up. As such we modify these confusing include directives and prefix the omr directory so it is very clear. Signed-off-by: Filip Jeremic <[email protected]>

Signed-off-by: Filip Jeremic <[email protected]>

fjeremic · 2018-05-30T20:54:31Z

@mstoodle rebase was clean and with no merge conflicts. We should be ready to merge here.

fjeremic added comp:jit comp:build depends:omr Pull request is dependent on a corresponding change in OMR labels May 25, 2018

fjeremic force-pushed the deprecate-trj9 branch from 670b698 to cc6f59c Compare May 25, 2018 03:54

fjeremic assigned mstoodle May 25, 2018

fjeremic force-pushed the deprecate-trj9 branch from 29f5844 to b6fea32 Compare May 25, 2018 18:48

mstoodle approved these changes May 25, 2018

View reviewed changes

fjeremic force-pushed the deprecate-trj9 branch from b6fea32 to 40f541d Compare May 28, 2018 18:09

dnakamura mentioned this pull request May 28, 2018

CMake: remove use of J9SRC and OMR_ROOT from compiler #2002

Merged

fjeremic force-pushed the deprecate-trj9 branch from 40f541d to 31a56dd Compare May 28, 2018 21:01

fjeremic force-pushed the deprecate-trj9 branch from 31a56dd to de6be3e Compare May 29, 2018 14:16

fjeremic added 8 commits May 30, 2018 16:50

Rename all include directives to eliminate trj9

67f48c4

Now that all files have been moved from compiler/trj9 to compiler rename the include directives appropriately. Signed-off-by: Filip Jeremic <[email protected]>

Rename trj9 to compiler in the makefiles

a98f068

Signed-off-by: Filip Jeremic <[email protected]>

Update DDR and verbose module metadata w.r.t. trj9

71e8a18

Signed-off-by: Filip Jeremic <[email protected]>

Consolidate compiler makefiles

bb9b78e

When the build system processes makefile.ftl it creates a file called makefile, thus we end up having both makefile and Makefile. This is unnecessary and these files can be consolidated. Signed-off-by: Filip Jeremic <[email protected]>

Update JIT_SRCBASE after folding trj9 directory

5b475b8

Signed-off-by: Filip Jeremic <[email protected]>

fjeremic force-pushed the deprecate-trj9 branch from de6be3e to 5b475b8 Compare May 30, 2018 20:53

mstoodle merged commit dfc1646 into eclipse-openj9:master May 30, 2018

fjeremic deleted the deprecate-trj9 branch August 30, 2018 15:59

fjeremic mentioned this pull request Nov 19, 2018

A number of compiler sources assume that omr is a subdirectory of openj9 #3725

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate trj9 compiler directory #1992

Deprecate trj9 compiler directory #1992

fjeremic commented May 25, 2018 •

edited

Loading

fjeremic commented May 25, 2018

fjeremic commented May 25, 2018

fjeremic commented May 25, 2018

fjeremic commented May 25, 2018

mstoodle commented May 25, 2018 •

edited

Loading

fjeremic commented May 25, 2018

fjeremic commented May 25, 2018

mstoodle commented May 25, 2018 •

edited

Loading

mstoodle left a comment

AdamBrousseau commented May 25, 2018

mstoodle commented May 25, 2018 •

edited

Loading

mstoodle commented May 28, 2018 •

edited

Loading

fjeremic commented May 28, 2018

AdamBrousseau commented May 28, 2018 •

edited

Loading

fjeremic commented May 28, 2018 •

edited

Loading

fjeremic commented May 28, 2018 •

edited

Loading

fjeremic commented May 28, 2018

fjeremic commented May 28, 2018

fjeremic commented May 28, 2018

fjeremic commented May 29, 2018

fjeremic commented May 30, 2018 •

edited

Loading

pshipton commented May 30, 2018

fjeremic commented May 30, 2018

pshipton commented May 30, 2018

fjeremic commented May 30, 2018

fjeremic commented May 30, 2018

Deprecate trj9 compiler directory #1992

Deprecate trj9 compiler directory #1992

Conversation

fjeremic commented May 25, 2018 • edited Loading

fjeremic commented May 25, 2018

fjeremic commented May 25, 2018

fjeremic commented May 25, 2018

fjeremic commented May 25, 2018

mstoodle commented May 25, 2018 • edited Loading

fjeremic commented May 25, 2018

fjeremic commented May 25, 2018

mstoodle commented May 25, 2018 • edited Loading

mstoodle left a comment

Choose a reason for hiding this comment

AdamBrousseau commented May 25, 2018

mstoodle commented May 25, 2018 • edited Loading

mstoodle commented May 28, 2018 • edited Loading

fjeremic commented May 28, 2018

AdamBrousseau commented May 28, 2018 • edited Loading

fjeremic commented May 28, 2018 • edited Loading

fjeremic commented May 28, 2018 • edited Loading

fjeremic commented May 28, 2018

fjeremic commented May 28, 2018

fjeremic commented May 28, 2018

fjeremic commented May 29, 2018

fjeremic commented May 30, 2018 • edited Loading

pshipton commented May 30, 2018

fjeremic commented May 30, 2018

pshipton commented May 30, 2018

fjeremic commented May 30, 2018

fjeremic commented May 30, 2018

fjeremic commented May 25, 2018 •

edited

Loading

mstoodle commented May 25, 2018 •

edited

Loading

mstoodle commented May 25, 2018 •

edited

Loading

mstoodle commented May 25, 2018 •

edited

Loading

mstoodle commented May 28, 2018 •

edited

Loading

AdamBrousseau commented May 28, 2018 •

edited

Loading

fjeremic commented May 28, 2018 •

edited

Loading

fjeremic commented May 28, 2018 •

edited

Loading

fjeremic commented May 30, 2018 •

edited

Loading