TS-38628 Rewrite environment path installing step

installer/src/main/kotlin/com/teamscale/profiler/installer/Installer.kt

@@ -36,7 +36,7 @@ class Installer(
 	 * permissions.
 	 */
 	init {
-		val environmentVariables = getEnvironmentVariables(installDirectory)
+		val environmentVariables = installDirectory.environmentVariables()
 		steps = listOf(
 			InstallAgentFilesStep(sourceDirectory, installDirectory),
 			InstallWindowsSystemEnvironmentStep(environmentVariables, registry),
@@ -98,10 +98,9 @@ class Installer(
 	 *  * JAVA_TOOL_OPTIONS is recognized by all JVMs but may be overridden by application start scripts
 	 *  * _JAVA_OPTIONS is not officially documented but currently well-supported and unlikely to be used
 	 * by application start scripts
-	 *
 	 */
-	private fun getEnvironmentVariables(installDirectory: Path): JvmEnvironmentMap {
-		val javaAgentArgument = "-javaagent:" + getAgentJarPath(installDirectory)
+	private fun Path.environmentVariables(): JvmEnvironmentMap {
+		val javaAgentArgument = "-javaagent:" + getAgentJarPath(this)
 		return JvmEnvironmentMap(
 			"JAVA_TOOL_OPTIONS", javaAgentArgument,
 			"_JAVA_OPTIONS", javaAgentArgument

installer/src/main/kotlin/com/teamscale/profiler/installer/steps/InstallEtcEnvironmentStep.kt

@@ -8,70 +8,119 @@ import java.nio.charset.StandardCharsets
 import java.nio.file.Files
 import java.nio.file.Path
 import java.nio.file.StandardOpenOption
-import java.util.stream.Collectors
+import kotlin.io.path.notExists
+import kotlin.io.path.readLines
 
-/** On Linux, registers the agent globally via environment variables set in /etc/environment  */
+/**
+ * On Linux, registers the agent globally via environment variables set in /etc/environment.
+ */
 class InstallEtcEnvironmentStep(
 	private val etcDirectory: Path,
 	private val environmentVariables: JvmEnvironmentMap
 ) : IStep {
-	override fun shouldRun() = SystemUtils.IS_OS_LINUX
 
+	override fun shouldRun(): Boolean = SystemUtils.IS_OS_LINUX
+
+	private val environmentFile: Path by lazy {
+		etcDirectory.resolve("environment")
+	}
+
+	/**
+	 * Installs the required environment variables in the /etc/environment file.
+	 * If the file doesn't exist, it logs a warning and avoids further modification.
+	 */
 	@Throws(FatalInstallerError::class)
 	override fun install(credentials: TeamscaleCredentials) {
-		val environmentFile = environmentFile
-		val etcEnvironmentAddition = java.lang.String.join("\n", environmentVariables.etcEnvironmentLinesList)
-
-		if (!Files.exists(environmentFile)) {
-			System.err.println(
-				"""$environmentFile does not exist. Skipping system-wide registration of the profiler.
-				You need to manually register the profiler for process that should be profiled by setting the following environment variables:
-				
-				$etcEnvironmentAddition
-				""".trimIndent()
-			)
+		if (environmentFile.notExists()) {
+			logMissingEnvironmentFile()
 			return
 		}
 
-		val content = """
-			$etcEnvironmentAddition
-			""".trimIndent()
 		try {
-			Files.writeString(
-				environmentFile, content, StandardCharsets.US_ASCII,
-				StandardOpenOption.APPEND
-			)
+			val existingLines = Files.readAllLines(environmentFile, StandardCharsets.UTF_8)
+			val newLines = generateNewEnvironmentLines(existingLines)
+
+			if (newLines.isNotEmpty()) {
+				Files.write(
+					environmentFile,
+					newLines,
+					StandardCharsets.UTF_8,
+					StandardOpenOption.APPEND
+				)
+			}
+
 		} catch (e: IOException) {
-			throw PermissionError("Could not change contents of $environmentFile", e)
+			throw PermissionError("Failed to modify ${environmentFile.toAbsolutePath()}.", e)
 		}
 	}
 
-	private val environmentFile: Path
-		get() = etcDirectory.resolve("environment")
-
+	/**
+	 * Uninstalls any previously added environment variables from the /etc/environment file.
+	 */
 	override fun uninstall(errorReporter: IUninstallErrorReporter) {
-		val environmentFile = environmentFile
-		if (!Files.exists(environmentFile)) {
-			return
-		}
+		if (environmentFile.notExists()) return
 
 		try {
-			val lines = Files.readAllLines(environmentFile, StandardCharsets.US_ASCII)
-			val newContent = removeProfilerVariables(lines)
-			Files.writeString(environmentFile, newContent, StandardCharsets.US_ASCII)
+			val existingLines = Files.readAllLines(environmentFile, StandardCharsets.UTF_8)
+			val cleanedLines = removeProfilerEntries(existingLines)
+
+			Files.write(
+				environmentFile,
+				cleanedLines,
+				StandardCharsets.UTF_8,
+				StandardOpenOption.TRUNCATE_EXISTING
+			)
 		} catch (e: IOException) {
 			errorReporter.report(
 				PermissionError(
-					"Failed to remove profiler from " + environmentFile + "." +
-							" Please remove the relevant environment variables yourself." +
-							" Otherwise, Java applications may crash.", e
+					"Failed to remove profiler entries from ${environmentFile.toAbsolutePath()}. " +
+							"Please manually verify and clean up the file to avoid issues.",
+					e
 				)
 			)
 		}
 	}
 
-	private fun removeProfilerVariables(linesWithoutNewline: List<String>) =
-		linesWithoutNewline.filter { line ->
-			!environmentVariables.etcEnvironmentLinesList.contains(line)
-		}.joinToString("\n")
-}
+	/**
+	 * Logs a warning when the /etc/environment file does not exist.
+	 */
+	private fun logMissingEnvironmentFile() {
+		System.err.println(
+			"""
+            WARNING: ${environmentFile.toAbsolutePath()} does not exist. Skipping system-wide registration of the profiler.
+                     You need to manually register the profiler for processes that should be profiled by setting the 
+                     following environment variables:
+                     
+                     ${environmentVariables.etcEnvironmentLinesList.joinToString("\n")}
+            """.trimIndent()
+		)
+	}
+
+	/**
+	 * Appends the environment variables only if they don't already exist in the file.
+	 */
+	private fun generateNewEnvironmentLines(existingLines: List<String>): List<String> {
+		val newEntries = environmentVariables.etcEnvironmentLinesList.filter { newEntry ->
+			// Only include the new entry if it doesn't exist in the current environment file
+			existingLines.none { it.contains(newEntry) }
+		}
+
+		return if (newEntries.isNotEmpty()) {
+			listOf("\n") + newEntries // Add an empty line for separation before appending entries
+		} else {
+			emptyList()
+		}
+	}
+
+	/**
+	 * Removes the profiler variables from an existing list of lines.
+	 * Handles cases where variables are merged improperly into other environment settings.
+	 */
+	private fun removeProfilerEntries(existingLines: List<String>) =
+		existingLines.filter { line ->
+			environmentVariables.etcEnvironmentLinesList.none { variable ->
+				// Ensure variable is completely absent in the existing line
+				line.contains(variable)
+			}
+		}
+}

installer/src/test/kotlin/com/teamscale/profiler/installer/LinuxInstallerTest.kt

@@ -1,6 +1,5 @@
 package com.teamscale.profiler.installer
 
-import com.teamscale.profiler.installer.Installer.UninstallerErrorReporter
 import com.teamscale.profiler.installer.utils.MockTeamscale
 import com.teamscale.profiler.installer.utils.TestUtils
 import com.teamscale.profiler.installer.utils.UninstallErrorReporterAssert
@@ -16,8 +15,8 @@ import org.junit.jupiter.api.condition.OS
 import java.io.IOException
 import java.nio.file.Files
 import java.nio.file.Path
-import java.nio.file.StandardOpenOption
 import java.nio.file.attribute.PosixFilePermission
+import kotlin.io.path.*
 
 @EnabledOnOs(OS.LINUX)
 internal class LinuxInstallerTest {
@@ -36,25 +35,27 @@ internal class LinuxInstallerTest {
 	@BeforeEach
 	@Throws(IOException::class)
 	fun setUpSourceDirectory() {
-		sourceDirectory = Files.createTempDirectory("InstallerTest-source")
-		targetDirectory = Files.createTempDirectory("InstallerTest-target").resolve("profiler")
-		etcDirectory = Files.createTempDirectory("InstallerTest-etc")
+		sourceDirectory = createTempDirectory("InstallerTest-source")
+		targetDirectory = createTempDirectory("InstallerTest-target").resolve("profiler")
+		etcDirectory = createTempDirectory("InstallerTest-etc")
 
 		environmentFile = etcDirectory.resolve("environment")
-		Files.writeString(environmentFile, ENVIRONMENT_CONTENT)
+		environmentFile.writeText(ENVIRONMENT_CONTENT)
 
 		systemdDirectory = etcDirectory.resolve("systemd")
-		Files.createDirectory(systemdDirectory)
+		systemdDirectory.createDirectory()
 		systemdConfig = systemdDirectory.resolve("system.conf.d/teamscale-java-profiler.conf")
 
-		val fileToInstall = sourceDirectory.resolve("install-me.txt")
-		Files.writeString(fileToInstall, FILE_TO_INSTALL_CONTENT, StandardOpenOption.CREATE)
+		val fileToInstall = sourceDirectory.resolve("install-me.txt").apply {
+			writeText(FILE_TO_INSTALL_CONTENT)
+		}
 
-		val nestedFileToInstall = sourceDirectory.resolve("lib/teamscale-jacoco-agent.jar")
-		Files.createDirectories(nestedFileToInstall.parent)
-		Files.writeString(nestedFileToInstall, NESTED_FILE_CONTENT, StandardOpenOption.CREATE)
+		sourceDirectory.resolve("lib/teamscale-jacoco-agent.jar").apply {
+			parent.createDirectories()
+			writeText(NESTED_FILE_CONTENT)
+		}
 
-		installedFile = targetDirectory.resolve(sourceDirectory.relativize(fileToInstall))
+		installedFile = targetDirectory.resolve(fileToInstall.relativeTo(sourceDirectory))
 		installedTeamscaleProperties = targetDirectory.resolve("teamscale.properties")
 		installedAgentLibrary = targetDirectory.resolve("lib/teamscale-jacoco-agent.jar")
 	}
@@ -93,7 +94,7 @@ internal class LinuxInstallerTest {
 		UninstallErrorReporterAssert.assertThat(errorReporter).hadNoErrors()
 
 		Assertions.assertThat(targetDirectory).doesNotExist()
-		Assertions.assertThat(environmentFile).exists().content().isEqualTo(ENVIRONMENT_CONTENT)
+		Assertions.assertThat(environmentFile).exists().content().isEqualToIgnoringWhitespace(ENVIRONMENT_CONTENT)
 		Assertions.assertThat(systemdConfig).doesNotExist()
 	}
 
@@ -127,7 +128,7 @@ internal class LinuxInstallerTest {
 
 		Assertions.assertThat(targetDirectory).exists()
 		Assertions.assertThat(installedTeamscaleProperties).exists()
-		Assertions.assertThat(environmentFile).exists().content().isEqualTo(ENVIRONMENT_CONTENT)
+		Assertions.assertThat(environmentFile).exists().content().isEqualToIgnoringWhitespace(ENVIRONMENT_CONTENT)
 		Assertions.assertThat(systemdConfig).doesNotExist()
 	}