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

Add --pkg-config-path to LocationOptions #5949

Merged
merged 10 commits into from
Dec 19, 2022
Merged

Add --pkg-config-path to LocationOptions #5949

merged 10 commits into from
Dec 19, 2022

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Dec 6, 2022 •
edited
Loading

Resolves #5815.

Motivation:

--pkg-config-path option is less fragile and easier to report errors on than PKG_CONFIG_PATH environment variable.

Modifications:

Added pkgConfigDirectory to LocationOptions, which is propagated to BuildPlan as a function or initializer argument.

Result:

--pkg-config-path is available as a command-line option.

@MaxDesiatov MaxDesiatov self-assigned this Dec 6, 2022
tomerd
tomerd reviewed Dec 6, 2022
View reviewed changes
Sources/Build/BuildOperation.swift Outdated Show resolved Hide resolved
Sources/CoreCommands/SwiftTool.swift Outdated Show resolved Hide resolved
Sources/PackageLoading/Target+PkgConfig.swift Show resolved Hide resolved
Sources/XCBuildSupport/PIFBuilder.swift Outdated Show resolved Hide resolved
@neonichu
Copy link
Contributor

neonichu commented Dec 6, 2022

I believe PKG_CONFIG_PATH is a list of paths, so if we want to provide an alternative to it, we should probably also take a list here.

If I read this correctly, it is also not being passed to XCBuild support, yet, we should do that.

@MaxDesiatov
Copy link
Contributor Author

I believe PKG_CONFIG_PATH is a list of paths, so if we want to provide an alternative to it, we should probably also take a list here.

Would it make sense to make them comma-separated? I wonder if just changing the type on the option declaration to [AbsolutePath] would work with some built-in support from ArgumentParser?

If I read this correctly, it is also not being passed to XCBuild support, yet, we should do that.

Is there anything to adjust other than this place in PIFBuilder.swift? https://github.com/apple/swift-package-manager/pull/5949/files#diff-4558ab2f4d59c923d54773a355350f9362890d5b982aaf79ae5854f8e93eb09fR703

@neonichu
Copy link
Contributor

neonichu commented Dec 7, 2022

Would it make sense to make them comma-separated? I wonder if just changing the type on the option declaration to [AbsolutePath] would work with some built-in support from ArgumentParser?

Not sure, but we should do whatever is the most canonical way to do this. Maybe @natecook1000 has hints

@MaxDesiatov MaxDesiatov force-pushed the maxd/pkg-config-path-option branch from 5767258 to bc0a809 Compare December 14, 2022 15:07
@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov MaxDesiatov marked this pull request as ready for review December 15, 2022 17:54
@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Dec 15, 2022 •
edited
Loading

Would it make sense to make them comma-separated? I wonder if just changing the type on the option declaration to [AbsolutePath] would work with some built-in support from ArgumentParser?

Not sure, but we should do whatever is the most canonical way to do this.

@neonichu turns out, specifying it as [AbsolutePath] allows passing the option multiple times. I've also added tests and updated CHANGELOG.md, this is now ready for review.

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov
Merge branch 'main' of github.com:apple/swift-package-manager into ma…
60943b1 
…xd/pkg-config-path-option

# Conflicts:
#	Sources/CoreCommands/Options.swift
#	Sources/CoreCommands/SwiftTool.swift
@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@finagolfin
Copy link
Member

I was hoping we'd get this in before the 5.8 branch tomorrow, still possible?

@MaxDesiatov
Copy link
Contributor Author

I'm not sure, it depends on whether anyone is available for a review.

@MaxDesiatov MaxDesiatov requested a review from tomerd December 19, 2022 10:04
@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) December 19, 2022 20:02
@MaxDesiatov MaxDesiatov merged commit 1863e35 into main Dec 19, 2022
tomerd
tomerd reviewed Dec 21, 2022
View reviewed changes
CHANGELOG.md
@@ -3,6 +3,13 @@ Note: This is in reverse chronological order, so newer entries are added to the
Swift 5.8
-----------

* [#5949]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should sort these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already in reverse chronological order, or would you like to see some other sorting?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the rest of the document has is non-reverse chronological order?

@MaxDesiatov MaxDesiatov deleted the maxd/pkg-config-path-option branch December 22, 2022 14:32
@finagolfin
Copy link
Member

I can confirm that this flag works when cross-compiling SPM for Android with the latest 5.8 Dec. 20 snapshot, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomerd tomerd tomerd left review comments

@neonichu neonichu neonichu approved these changes

@abertelrud abertelrud Awaiting requested review from abertelrud

@elsh elsh Awaiting requested review from elsh

Assignees

@MaxDesiatov MaxDesiatov

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Time to graduate PKG_CONFIG_PATH to a command-line option?
4 participants
@MaxDesiatov @neonichu @finagolfin @tomerd