Skip to content

refactor authorization provider setup #4074

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

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Feb 1, 2022

motivation: more easily share authorization provider setup with libSwiftPM users

changes:

  • move authorization providers setup from SwiftTool to a Worksapce configuraiton utility
  • adjust call sites and tests

@tomerd tomerd requested a review from yim-lee February 1, 2022 20:11
@tomerd
Copy link
Contributor Author

tomerd commented Feb 1, 2022

@swift-ci please smoke test

if let userHomeProvider = loadNetrcNoThrows(at: userHomePath) {
providers.append(userHomeProvider)
}
let rootPath = try options.multirootPackageDataFile ?? self.getPackageRoot()
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just moving the code, but I am not sure if it this is actually correct for multi root. This will be a file path, not a directory, so essentially never work.

Copy link
Contributor Author

@tomerd tomerd Feb 1, 2022

Choose a reason for hiding this comment

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

interesting, the entire code in SwiftTool treats multirootPackageDataFile as the root directory, sounds like that's not working (and that we are missing some tests?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see how it works, options.multirootPackageDataFile is actually pointing to a .xcworkspace which is a directory. I think we should change this to use xcshareddata because this is a configuration file.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, what I mean is use multiRootPackageDataFile.appending(components: "xcshareddata") as the root path here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds like good material for follow on PR? it is used in various spots

@tomerd tomerd force-pushed the refactor/authorization-provider branch from 36f72ef to 9307725 Compare February 1, 2022 23:52
@tomerd
Copy link
Contributor Author

tomerd commented Feb 1, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Feb 2, 2022

@swift-ci please smoke test

@tomerd tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Feb 2, 2022
@tomerd tomerd self-assigned this Feb 2, 2022
@tomerd tomerd force-pushed the refactor/authorization-provider branch from d7aa798 to 074eff2 Compare February 2, 2022 19:53
@tomerd
Copy link
Contributor Author

tomerd commented Feb 2, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Feb 3, 2022

lets get #4079 in first, then we can rebase this one on top

@tomerd tomerd force-pushed the refactor/authorization-provider branch from 7e50f47 to 8a7d883 Compare February 3, 2022 19:49
@tomerd
Copy link
Contributor Author

tomerd commented Feb 3, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Feb 3, 2022

@yim-lee @neonichu this is ready for review

Copy link
Contributor

@yim-lee yim-lee left a comment

Choose a reason for hiding this comment

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

We should probably run source compat since we've had problems with related code this week.

@tomerd tomerd force-pushed the refactor/authorization-provider branch 2 times, most recently from 04ca6df to b454f30 Compare February 4, 2022 04:54
motivation: more easily share authorization provider setup with libSwiftPM users

changes:
* move authorization providers setup from SwiftTool to a Worksapce configuraiton utility
* adjust call sites and tests
@tomerd tomerd force-pushed the refactor/authorization-provider branch from b454f30 to a0011b2 Compare February 4, 2022 04:56
@tomerd
Copy link
Contributor Author

tomerd commented Feb 4, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Feb 5, 2022

source compat looks good: swiftlang/swift#41142

i think this is good to go. @yim-lee want to take one last look?

@yim-lee
Copy link
Contributor

yim-lee commented Feb 5, 2022

LGTM 👍

@tomerd tomerd merged commit 3fdd7d3 into swiftlang:main Feb 7, 2022
@tomerd
Copy link
Contributor Author

tomerd commented Feb 7, 2022

@neonichu this is now merged so can be used downstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants