Skip to content

Fix -fmodule-map-file pointing to non-existent path #3247

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

Conversation

neonichu
Copy link
Contributor

@neonichu neonichu commented Feb 8, 2021

We were passing this unconditionally which would break in the case where we weren't generated a module map because an explicit one exists.

rdar://74115399

@neonichu
Copy link
Contributor Author

neonichu commented Feb 8, 2021

@swift-ci please smoke test

@neonichu neonichu requested a review from yim-lee February 8, 2021 22:51
@neonichu
Copy link
Contributor Author

neonichu commented Feb 8, 2021

Tested locally to verify that this fixes the issue mentioned in #3246

Copy link
Contributor

@abertelrud abertelrud left a comment

Choose a reason for hiding this comment

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

The change LGTM. Does this have a unit test that should be extended?

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.

I verified that this fixes the error I was seeing locally. Need to wait for CI (swiftlang/swift#35202) to see if it fixes the build issue we saw with the source compat suite in swiftlang/swift#35503.

@neonichu
Copy link
Contributor Author

neonichu commented Feb 8, 2021

We should probably add a test, I was just trying to have a PR up quickly.

@tomerd
Copy link
Contributor

tomerd commented Feb 8, 2021

/Users/buildnode/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm/Tests/XCBuildSupportTests/PIFBuilderTests.swift:1248: error: -[XCBuildSupportTests.PIFBuilderTests testLibraryTargets] : XCTAssertEqual failed: ("nil") is not equal to ("Optional(["$(inherited)", "-fmodule-map-file=$(OBJROOT)/GeneratedModuleMaps/$(PLATFORM_NAME)/FooLib1.modulemap"])")

@neonichu looks like a test needs to be updated?

@neonichu
Copy link
Contributor Author

neonichu commented Feb 9, 2021

Looks like the test is correct, but the fix isn't. We still need to impart for Swift targets. The only case where we shouldn't is for clang targets with an explicit module map.

We were passing this unconditionally which would break in the case where we weren't generated a module map because an explicit one exists.

rdar://74115399
@neonichu neonichu force-pushed the only-pass-module-map-explicitly-if-generated branch from af0ad46 to 9a2fdbc Compare February 9, 2021 00:31
@neonichu
Copy link
Contributor Author

neonichu commented Feb 9, 2021

@swift-ci please smoke test

Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

code changes seems reasonable, not familiar with the business logic tho

Copy link
Contributor

@abertelrud abertelrud left a comment

Choose a reason for hiding this comment

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

Looks good to me, and agree that a test can follow in a separate PR in the interest of quickly fixing this.

@yim-lee
Copy link
Contributor

yim-lee commented Feb 9, 2021

@neonichu neonichu merged commit fcbb67c into swiftlang:main Feb 9, 2021
@neonichu neonichu deleted the only-pass-module-map-explicitly-if-generated branch February 9, 2021 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants