Devroot cleanup #2974
Devroot cleanup #2974
Conversation
| devroot = args[0]; | ||
| } else if (args.length == 2) { | ||
| devroot = args[0]; | ||
| assertNoChange = Boolean.parseBoolean(args[1]); | ||
| } else { | ||
| System.out.println("Usage: [<devroot> [assertNoChange]]"); | ||
| System.out.println("Usage: <devroot> [assertNoChange]"); |
There was a problem hiding this comment.
Have you tested all of the generators?
There was a problem hiding this comment.
I've run ./gradlew Generators:generateAll and I think testing should also assert all the generators are correct.
| .getProperty("PythonDeephavenSession.defaultScriptPath") | ||
| .replace("<devroot>", Configuration.getInstance().getDevRootPath()) | ||
| .replace("<workspace>", Configuration.getInstance().getWorkspacePath()); | ||
| .getStringWithDefault("PythonDeephavenSession.defaultScriptPath", "."); |
There was a problem hiding this comment.
Are we confident about making . the new default for all the script sessions? What testing have you done?
There was a problem hiding this comment.
This previous defaults were <X>.defaultScriptPath=<devroot>; and by default, devroot=.. So in theory, it's the same defaults as before, just a layer of indirection removed.
I think the defaultScriptPath logic could use serious reworking - I'm not sure it makes sense anymore if we have a better way w/ application mode. There's a comment to this effect in io.deephaven.engine.util.ScriptFinder#findScriptEx
I've run locally natively in python and groovy. I'll do testing in docker as well. I'll start a nightly as well.
There was a problem hiding this comment.
Everything I've run seems to work. nightly is passing except I hit this #2732
There was a problem hiding this comment.
Also run python embedded server, work fine.
| .getProperty("PythonDeephavenSession.defaultScriptPath") | ||
| .replace("<devroot>", Configuration.getInstance().getDevRootPath()) | ||
| .replace("<workspace>", Configuration.getInstance().getWorkspacePath()); | ||
| .getStringWithDefault("PythonDeephavenSession.defaultScriptPath", "."); |
There was a problem hiding this comment.
Related to this change:
io.deephaven.engine.util.ScriptFinder#findScriptEx(java.lang.String, java.lang.String) has some comments that probably need cleanup after devroot is retired.
There was a problem hiding this comment.
Ha! Just mentioned this in the previous comment before I noticed this.
This removes the necessity for the
devrootproperty to be set - it doesn't make sense for end-users. In code-generation or test cases where access to the source is still necessary, the appropriate system properties can be set as necessary.