Fix environment related warnings #7684

rauhul · 2024-06-18T21:31:28Z

Adds a new Environment type to replace TSCBasics EnvironmentVariables
and ProcessEnvironmentBlock types. Updates points of use of the older
APIs with the newer API and adds unit tests for Environment.

MaxDesiatov

I'm not convinced Environment warrants its own module, I'd prefer to keep it in Basics for now. It's unclear to me how that separation logic should be followed if at all. There's a lot of functionality in Basics, a new module for each type (or functionality aspect) would require a dozen of new modules, which IMO overcomplicates things.

MaxDesiatov · 2024-06-19T09:36:12Z

If you insist on moving it out of Basics, it could be kept in Basics here as a stopgap measure. After AsyncProcess PR is merged, Environment could be moved to that new AsyncProcess module.

xedin · 2024-06-19T17:13:52Z

It seems like AsyncProcess module is indeed a good place for this type.

MaxDesiatov · 2024-06-19T17:24:07Z

Main problem with that is a dependency on AbsolutePath in the AsyncProcess type itself. For the sake of not opening that can of modularization worms, I'd prefer this PR to be focused on fixing the actual warnings only. We can come back to modularization issues separately. This would also make PRs smaller and easier to review.

rauhul · 2024-06-19T23:38:59Z

@swift-ci test

rauhul · 2024-06-19T23:53:03Z

@swift-ci test windows

MaxDesiatov · 2024-06-20T00:01:13Z

@compnerd would you have a moment to look how well this works for Windows compatibility?

compnerd · 2024-06-20T00:08:02Z

Sources/Basics/Environment/Environment.swift

+    }
+}
+
+// FIXME: This should come from Foundation


I don't think that this can come from Foundation. Foundation's API is not easy to change and the environment being case insensitive is not currently well handled there.

The intent of the comment that this type should come from a common Swift ecosystem library and foundation seems like the most natural place.

compnerd · 2024-06-20T00:09:28Z

Sources/Basics/Environment/Environment.swift

+        }
+    }
+
+    package static var pathValueDelimiter: String {


I think pathEntryDelimiter is better - the variable is a set of key, value pairs which makes value a bit overloaded.

Yeah, I can change the name to your suggestion

compnerd · 2024-06-20T00:11:45Z

Sources/Basics/Environment/Environment.swift

+    package static func set(key: EnvironmentKey, value: String?) throws {
+        if let value = value {
+            #if os(Windows)
+            guard _putenv("\(key)=\(value)") == 0 else {


This is annoying - I don't know if we should use _putenv or SetEnvironmentVariableW. The former only adjusts the "C" functions view, the latter only changes the Win32 view. I suppose that we could call both?

I can have it call both, in general I strongly dislike mutating the global environment anyways

compnerd · 2024-06-20T00:13:56Z

Sources/Basics/Environment/Environment.swift

+        for (key, value) in sorted {
+            if first {
+                first = false
+            } else {
+                description.append(", ")
+            }
+            description.append("\"")
+            description.append(key.rawValue)
+            description.append("=")
+            description.append(value)
+            description.append("\"")
+        }


Suggested change

for (key, value) in sorted {

if first {

first = false

} else {

description.append(", ")

}

description.append("\"")

description.append(key.rawValue)

description.append("=")

description.append(value)

description.append("\"")

}

sorted.map { #""\#($0.rawValue)"="\#($1)""# }.joined(", ")

I avoided string interpolation and join to avoid intermediate allocations

That said it really doesn't matter and I can use an easier to read method

Curious if we have issues on the compiler repo tracking possible optimizer work that hypothetically could get rid of intermediate allocations here?

resolve macOS CI compiler crash

MaxDesiatov

Thank you for making SwiftPM more environment friendly! 🙈

MaxDesiatov · 2024-06-20T23:28:52Z

@swift-ci test windows

rauhul · 2024-06-20T23:43:40Z

@swift-ci test

rauhul · 2024-06-20T23:46:04Z

@swift-ci test windows

rauhul · 2024-06-21T00:00:35Z

@swift-ci test windows

rauhul · 2024-06-21T01:11:50Z

looks like windows CI is failing to checkout Swift which is odd because the other checks are working

rauhul · 2024-06-21T02:12:35Z

@swift-ci test windows

MaxDesiatov · 2024-06-21T08:58:00Z

@swift-ci test windows

xedin

Looks great, thank you! Could you please run swiftformat on new files before merging?

rauhul · 2024-06-21T18:55:56Z

@swift-ci test

rauhul · 2024-06-21T19:02:02Z

@swift-ci test windows

Adds a new `Environment` type to replace TSCBasics EnvironmentVariables and ProcessEnvironmentBlock types. Updates points of use of the older APIs with the newer API and adds unit tests for `Environment`. (cherry picked from commit 74aea87)

@xedin

Includes these PRs cherry-picked off `main` * #7605 * #7660 * #7667 * #7682 * #7687 * #7690 * #7684 * #7679 **Explanation**: Cherry-pick of recent NFC changes, which makes it easier to cherry-pick actual bug fixes onto 6.0 due to the reduced number of merge conflicts. **Scope**: broad, includes both modules graph and llbuild-related changes. **Risk**: low, the test suite is passing, no functional changes are included, and cherry-picked changes were incubated on `main` for some time. **Testing**: Existing automated test suite. **Issue**: N/A **Reviewers**: @xedin @MaxDesiatov @rauhul --------- Co-authored-by: Pavel Yaskevich <xedin@apache.org> Co-authored-by: Danny Mösch <danny.moesch@icloud.com> Co-authored-by: Rauhul Varma <rauhul@apple.com>

rauhul force-pushed the fix-warnings branch from 06ffeb7 to badd7f1 Compare June 18, 2024 22:40

MaxDesiatov requested changes Jun 19, 2024

View reviewed changes

rauhul force-pushed the fix-warnings branch from fae239d to 75ae741 Compare June 19, 2024 21:37

rauhul marked this pull request as ready for review June 19, 2024 21:40

rauhul requested review from bnbarham, jakepetroules, francescomikulis and xedin as code owners June 19, 2024 21:40

rauhul enabled auto-merge (squash) June 19, 2024 21:40

MaxDesiatov requested a review from compnerd June 19, 2024 23:59

compnerd reviewed Jun 20, 2024

View reviewed changes

rauhul added 7 commits June 20, 2024 16:07

Fix environment related warnings

c94e3cc

use newer environment type

ecbddad

Update CMakeLists

764c756

Move environment into Basics

1537114

Fix windows error

37b79e5

work around older swift bug

a3adf0e

review comments

fb3e1b9

resolve macOS CI compiler crash

rauhul force-pushed the fix-warnings branch from 90aecfc to fb3e1b9 Compare June 20, 2024 23:24

MaxDesiatov approved these changes Jun 20, 2024

View reviewed changes

MaxDesiatov requested a review from compnerd June 20, 2024 23:28

fix test failure

22bf47d

xedin approved these changes Jun 21, 2024

View reviewed changes

fix windows error and formatting

feb6ed1

rauhul merged commit 74aea87 into main Jun 21, 2024
5 checks passed

rauhul deleted the fix-warnings branch June 21, 2024 21:48

MaxDesiatov mentioned this pull request Jun 27, 2024

[6.0] Cherry-pick recent NFC changes to reduce merge conflicts #7721

Merged

rauhul mentioned this pull request Nov 20, 2024

Package plugins don't work on Windows if the PATH environment variable isn't accessed with the right case #8126

Merged

jakepetroules mentioned this pull request Feb 18, 2025

Add an EnvironmentBlock type to represent environment variables swiftlang/swift-build#186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix environment related warnings #7684

Fix environment related warnings #7684

rauhul commented Jun 18, 2024 •

edited

Loading

MaxDesiatov left a comment •

edited

Loading

MaxDesiatov commented Jun 19, 2024 •

edited

Loading

xedin commented Jun 19, 2024

MaxDesiatov commented Jun 19, 2024

rauhul commented Jun 19, 2024

rauhul commented Jun 19, 2024

MaxDesiatov commented Jun 20, 2024 •

edited

Loading

compnerd Jun 20, 2024

rauhul Jun 20, 2024

compnerd Jun 20, 2024

rauhul Jun 20, 2024

compnerd Jun 20, 2024

rauhul Jun 20, 2024

compnerd Jun 20, 2024

rauhul Jun 20, 2024

rauhul Jun 20, 2024

MaxDesiatov Jun 20, 2024

MaxDesiatov Jun 20, 2024

MaxDesiatov left a comment

MaxDesiatov commented Jun 20, 2024

rauhul commented Jun 20, 2024

rauhul commented Jun 20, 2024

rauhul commented Jun 21, 2024

rauhul commented Jun 21, 2024

rauhul commented Jun 21, 2024

MaxDesiatov commented Jun 21, 2024

xedin left a comment

rauhul commented Jun 21, 2024

rauhul commented Jun 21, 2024

Fix environment related warnings #7684

Fix environment related warnings #7684

Conversation

rauhul commented Jun 18, 2024 • edited Loading

MaxDesiatov left a comment • edited Loading

Choose a reason for hiding this comment

MaxDesiatov commented Jun 19, 2024 • edited Loading

xedin commented Jun 19, 2024

MaxDesiatov commented Jun 19, 2024

rauhul commented Jun 19, 2024

rauhul commented Jun 19, 2024

MaxDesiatov commented Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov left a comment

Choose a reason for hiding this comment

MaxDesiatov commented Jun 20, 2024

rauhul commented Jun 20, 2024

rauhul commented Jun 20, 2024

rauhul commented Jun 21, 2024

rauhul commented Jun 21, 2024

rauhul commented Jun 21, 2024

MaxDesiatov commented Jun 21, 2024

xedin left a comment

Choose a reason for hiding this comment

rauhul commented Jun 21, 2024

rauhul commented Jun 21, 2024

rauhul commented Jun 18, 2024 •

edited

Loading

MaxDesiatov left a comment •

edited

Loading

MaxDesiatov commented Jun 19, 2024 •

edited

Loading

MaxDesiatov commented Jun 20, 2024 •

edited

Loading