Skip to content

start external main processes#28

Merged
jurgenvinju merged 27 commits intomainfrom
start-external-main-processes
Mar 27, 2025
Merged

start external main processes#28
jurgenvinju merged 27 commits intomainfrom
start-external-main-processes

Conversation

@jurgenvinju
Copy link
Member

  • replaced use of lib scheme and removed superfluous code that created big classpaths for the JVM which were then again constructed inside of the REPL by PathConfig calling into mvn
  • fixed issues
  • set to the right SNAPSHOT version of rascal
  • started working on rewrite towards using main processes of tutor, compiler and checker

…big classpaths for the JVM which were then again constructed inside of the REPL by PathConfig calling into mvn
Watch out the snapshot dependency on rascal also needs to change to
"unshaded" again
…ascal dependency of the current project, or otherwise the rascal dependency of the maven plugin itself
Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

Looks good, but I think the stale detection logic needs a bit more configurability.

I do like how it cleans up the code.

command.add("-verbose");
}

return new ProcessBuilder(command).inheritIO().start();
Copy link
Member

Choose a reason for hiding this comment

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

The plexus-tools from maven has some dedicated tooling to invoke nexted commands: https://codehaus-plexus.github.io/plexus-utils/apidocs/org/codehaus/plexus/util/cli/Commandline.html

can we use that? (and let it take care of some of the messy work?)

Copy link
Member Author

Choose a reason for hiding this comment

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

noted. What messy work are you referring to? This is pretty straightforward stuff.


if (name.endsWith(".rsc")) {
return Set.of(
new File(targetDir, new File(file.getParentFile(), "$" + name.substring(0, name.length() - ".rsc".length()) + ".tpl").getPath())
Copy link
Member

Choose a reason for hiding this comment

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

in the case of the tutor or the repackager, this is not the right stale detection.

Copy link
Member Author

Choose a reason for hiding this comment

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

those (tutor and packager) do not use this code; but to make this more clear I will add a parameter to generalize it (in case we do use it in the future)


return builder.toString().substring(1);
}
public GenerateSourcesUsingRascalMojo(String mainClass, String skipTag) {
Copy link
Member

Choose a reason for hiding this comment

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

we also use generate sources for non rascal projects, so that looks like we'll loose that capability?

Copy link
Member Author

Choose a reason for hiding this comment

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

No this works as before. The RascalShell main still interprets its first commandline parameter as the module to load and run.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will test if it still works as before.

Copy link
Member Author

Choose a reason for hiding this comment

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

command.add(getRascalRuntime().toString());
		command.add(mainClass);
		if (mainModule != null && !mainModule.isEmpty()) {
			command.add(mainModule);
		}

Copy link
Member Author

Choose a reason for hiding this comment

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

That looks right.

}
// starts the processes asynchronously
for (int i = 1; i < chunks.size(); i++) {
processes.add(runMain(verbose, chunks.get(1), srcs, libs, tmpGeneratedSources.get(i), tmpBins.get(i)));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be chunks.get(1) but i?

And also, we should put the bin folder of the preCheck phase on the lib path of the parallel runner. so that it doesn't have to start all over again and recheck List etc.

Copy link
Member

Choose a reason for hiding this comment

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

also: since runMain uses inheritIO this will start "biting" each other.

It would be better to have the process allocate it's own streams, and then copy from them in a streaming way, making sure to lock before we write to our own streams. (like was happening in the original code, but it was doing this inside of the same JVM)

Copy link
Member Author

Choose a reason for hiding this comment

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

  • chunks.get(i): nice catch!
  • streams: yes that would be cool. We could also greatly simplify this and merge the output when everybody is done, without interleaving every line. I think I'd prefer that; it is much simpler and it is much easier to read.

Copy link
Member

Choose a reason for hiding this comment

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

yes, with the downside that while the typecheck is happening, you have no feedback. so it looks like it's stuck.

Copy link
Member Author

Choose a reason for hiding this comment

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

And also, we should put the bin folder of the preCheck phase on the lib path of the parallel runner. so that it doesn't have to start all over again and recheck List etc

nice catch!

@jurgenvinju jurgenvinju marked this pull request as ready for review March 27, 2025 11:32
@jurgenvinju jurgenvinju merged commit 274e30e into main Mar 27, 2025
0 of 3 checks passed
@jurgenvinju jurgenvinju deleted the start-external-main-processes branch March 27, 2025 12:57
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