Skip to content
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

Replace scala.sys.Process #5317

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

AndreiDreyer
Copy link
Contributor

@AndreiDreyer AndreiDreyer commented Feb 20, 2025

  • Replaced instances of scala.sys.Process with java.lang.ProcessBuilder
  • Moved io.joern.x2cpg.utils.ExternalCommand to io.shiftleft.semanticcpg.utils.ExternalCommand so it can be used in the required places
  • Added functionality to ExternalCommand that allows the process to run in the current directory inherited from the Java parent
  • Added ExternalCommandUtil, which replicates some of ExternalCommand.scala functionality so that it can be used in build.sbt files

@AndreiDreyer AndreiDreyer marked this pull request as ready for review February 20, 2025 14:20
@DavidBakerEffendi DavidBakerEffendi changed the title [WIP]: Replace scala.sys.Process Replace scala.sys.Process Feb 20, 2025
Copy link
Contributor

@max-leuthaeuser max-leuthaeuser left a comment

Choose a reason for hiding this comment

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

I don't quite get it sorry.

  • We now carry two almost identical implementation around. (semanticcpg and sbt)
  • User downstream need to change the dependency from x2cpg to semanticcpg or at it additionaly
  • Scala process is a wrapper around Java processbuilder anyway
  • Every invocation is now more complicated, e.g., if std out/err is not used and one only is interested in the return value
  • Especially at the sbt side of things simply using Scala process looks cleaner

@DavidBakerEffendi DavidBakerEffendi added infrastructure Modifications to build systems, CI/CD, or design consistency Concerns abstracting and adding tooling to unify duplicated or inconsistent code labels Feb 20, 2025
@@ -1,10 +1,11 @@
package io.joern.x2cpg.utils
package io.shiftleft.semanticcpg.utils
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to not make this a breaking change, make object ExternalCommand a trait or class and with e.g. an Impl suffix and have two objects extend it, one in the old and one in the new namespace. Please put a deprecation warning on the old namespace version. That way we dont break anything without a headsup.

…s.Process. Re-added io.joern.x2cpg.utils.ExternalCommand with Deprecated annotation. Added io.shiftleft.semanticcpg.utils.ExternalCommandImpl trait
…s.Process. Re-added io.joern.x2cpg.utils.ExternalCommand with Deprecated annotation. Added io.shiftleft.semanticcpg.utils.ExternalCommandImpl trait
@AndreiDreyer AndreiDreyer requested a review from ml86 February 21, 2025 09:36
Copy link
Collaborator

@DavidBakerEffendi DavidBakerEffendi left a comment

Choose a reason for hiding this comment

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

I'm happy if everyone else is happy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistency Concerns abstracting and adding tooling to unify duplicated or inconsistent code infrastructure Modifications to build systems, CI/CD, or design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants