Skip to content

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

Merged
MaxDesiatov merged 10 commits into
mainfrom
maxd/pkg-config-path-option
Dec 19, 2022
Merged

Add --pkg-config-path to LocationOptions#5949
MaxDesiatov merged 10 commits into
mainfrom
maxd/pkg-config-path-option

Conversation

@MaxDesiatov

@MaxDesiatov MaxDesiatov commented Dec 6, 2022

Copy link
Copy Markdown
Contributor

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
Comment thread Sources/Build/BuildOperation.swift Outdated
Comment thread Sources/CoreCommands/SwiftTool.swift Outdated
Comment thread Sources/PackageLoading/Target+PkgConfig.swift
Comment thread Sources/XCBuildSupport/PIFBuilder.swift Outdated
@neonichu

neonichu commented Dec 6, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

neonichu commented Dec 7, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

@swift-ci please smoke test

@MaxDesiatov

Copy link
Copy Markdown
Contributor Author

@swift-ci please smoke test

@MaxDesiatov

Copy link
Copy Markdown
Contributor Author

@swift-ci smoke test

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

MaxDesiatov commented Dec 15, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

@swift-ci smoke test

…xd/pkg-config-path-option

# Conflicts:
#	Sources/CoreCommands/Options.swift
#	Sources/CoreCommands/SwiftTool.swift
@MaxDesiatov

Copy link
Copy Markdown
Contributor Author

@swift-ci smoke test

@finagolfin

Copy link
Copy Markdown
Member

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

@MaxDesiatov

Copy link
Copy Markdown
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
Copy Markdown
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
Comment thread CHANGELOG.md
Swift 5.8
-----------

* [#5949]

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Time to graduate PKG_CONFIG_PATH to a command-line option?

4 participants