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.

Sign up for GitHub

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

Jump to bottom

enhance configuraiton for dependencies mirrors #3670

Merged
merged 2 commits into from
Aug 20, 2021
Merged

enhance configuraiton for dependencies mirrors #3670

merged 2 commits into from
Aug 20, 2021

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Aug 17, 2021

motivation: support both local and global configuraiton, as per SE-0292

changes:

  • reafactor and seperate out the mirrors data model from the loading/saving mutation utility
  • use Codable for serialization
  • add new utility to manage local and shared mirrors configuraiton files
  • add new CLI flag to set custom shared configuration path
  • adjust call sites
  • add tests

@tomerd tomerd force-pushed the refactor/configuration-mirrors branch from 0364556 to 077919f Compare August 17, 2021 05:19
@tomerd
Copy link
Contributor Author

tomerd commented Aug 17, 2021

cc @mattt @yim-lee this is to support the local + global config for mirrors. it also handles the migration from the current location to the new one per SE-0292

@tomerd
Copy link
Contributor Author

tomerd commented Aug 17, 2021

@swift-ci please smoke test

tomerd
tomerd commented Aug 17, 2021
View reviewed changes
Sources/Commands/SwiftTool.swift
try localFileSystem.createDirectory(newPath.parentDirectory, recursive: true)
try localFileSystem.move(from: legacyPath, to: newPath)
}
return newPath
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattt @neonichu @abertelrud note the code above does the migration from old path to new one. we should carefully integrate that into the clients such as Xcode

Copy link
Contributor

Choose a reason for hiding this comment

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

Among the other development-oriented documents in SwiftPM, maybe we should have one specifically focused on clients like IDEs that calls out these things. Or maybe a separate section in the change log. Just a thought.

@tomerd tomerd force-pushed the refactor/configuration-mirrors branch from 077919f to f49db9b Compare August 17, 2021 05:57
@tomerd
Copy link
Contributor Author

tomerd commented Aug 17, 2021

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Aug 17, 2021

@swift-ci please smoke test

@tomerd tomerd requested review from yim-lee and mattt August 17, 2021 07:31
mattt
mattt reviewed Aug 17, 2021
View reviewed changes
Copy link
Contributor

@mattt mattt left a comment

Choose a reason for hiding this comment

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

This is a nice improvement over what we have right now. I shared some ideas about the API design, but overall things are looking good.

@tomerd
Copy link
Contributor Author

tomerd commented Aug 17, 2021 

test/branch-main/build/buildbot_incremental/swiftpm-linux-x86_64/x86_64-unknown-linux-gnu/tsc/lib/libTSCLibc.so && :
01:20:38 
<unknown>:0: error: module file '/home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/build/buildbot_incremental/swiftpm-linux-x86_64/x86_64-unknown-linux-gnu/swift-driver/module-cache/1LLKE9K0333WU/CDispatch-3R896LDTEKJJR.pcm' is out of date and needs to be rebuilt: signature mismatch
01:20:38 <unknown>:0: note: imported by module '_SwiftDispatchOverlayShims' in '/home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/build/buildbot_incremental/swiftpm-linux-x86_64/x86_64-unknown-linux-gnu/swift-driver/module-cache/1LLKE9K0333WU/_SwiftDispatchOverlayShims-6NPB064P3NLH.pcm'
01:20:38 <unknown>:0: error: missing required module '_SwiftDispatchOverlayShims'
01:20:38 ninja: build stopped: subcommand failed.

@swift-ci please smoke test linux

@tomerd tomerd mentioned this pull request Aug 17, 2021
Merged
@tomerd tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Aug 17, 2021
@tomerd tomerd self-assigned this Aug 17, 2021
@tomerd
enhance configuraiton for dependencies mirrors
7fa4345 
motivation: support both local and global configuraiton, as per SE-0292

changes:
* reafactor and seperate out the mirrors data model from the loading/saving mutation utility
* use Codable for serialization
* add new utility to manage local and shared mirrors configuraiton files
* add new CLI flag to set custom shared configuration path
* adjust call sites
* add tests
@tomerd tomerd force-pushed the refactor/configuration-mirrors branch from 3fe1921 to fc30632 Compare August 19, 2021 17:04
@tomerd
Copy link
Contributor Author

tomerd commented Aug 19, 2021

@swift-ci please smoke test

@tomerd tomerd force-pushed the refactor/configuration-mirrors branch from fc30632 to dc0f251 Compare August 19, 2021 17:59
@tomerd
Copy link
Contributor Author

tomerd commented Aug 19, 2021

@swift-ci please smoke test

abertelrud
abertelrud reviewed Aug 19, 2021
View reviewed changes
Sources/Commands/SwiftTool.swift
@@ -499,43 +520,43 @@ public class SwiftTool {
throw ExitCode.failure
} else {
diagnostics.emit(warning: "Did not find optional .netrc file at \(resolvedPath.pathString).")
return nil
return .none
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed this change a couple of diffs. Is this the recommended style for conditionals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to use .none when the optional marks a clear functional decision, e.g. let cache = .none is more semantic in my mind then let cache = nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I'll start doing the same, and if/when we do a swift-format pass on the whole codebase we can do the rest then.

@tomerd
Copy link
Contributor Author

tomerd commented Aug 19, 2021 

<unknown>:0: error: module file '/home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/build/buildbot_incremental/llbuild-linux-x86_64/module-cache/1DC0LQ2OD9FIY/SwiftShims-3849312O4BV1I.pcm' is out of date and needs to be rebuilt: signature mismatch
12:00:29 <unknown>:0: note: imported by module 'SwiftOverlayShims' in '/home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/build/buildbot_incremental/llbuild-linux-x86_64/module-cache/1DC0LQ2OD9FIY/SwiftOverlayShims-3849312O4BV1I.pcm'
12:00:29 
/home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/llbuild/products/llbuildSwift/BuildSystemBindings.swift:17:8: error: missing required module 'SwiftOverlayShims'
12:00:29 import Glibc
12:00:29        ^
12:00:29 [140/1

@tomerd
Copy link
Contributor Author

tomerd commented Aug 19, 2021

@swift-ci please smoke test linux

@tomerd tomerd merged commit 50fc7f2 into swiftlang:main Aug 20, 2021
This was referenced Aug 23, 2021
Closed
Merged
mattt added a commit to swiftlang/swift-evolution that referenced this pull request Aug 30, 2021
@mattt
Replace .swiftpm/config with .swiftpm/configuration
7932803 
See swiftlang/swift-package-manager#3670
@mattt mattt mentioned this pull request Aug 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abertelrud abertelrud abertelrud approved these changes

@mattt mattt mattt left review comments

@friedbunny friedbunny Awaiting requested review from friedbunny

@neonichu neonichu Awaiting requested review from neonichu

@yim-lee yim-lee Awaiting requested review from yim-lee

Assignees

@tomerd tomerd

Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tomerd @mattt @neonichu @abertelrud