Skip to content

Fix build of host&target destination products with --static-swift-stdlib #7695

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

Conversation

kateinoigakukun
Copy link
Member

@kateinoigakukun kateinoigakukun commented Jun 21, 2024

Motivation:

Given the following conditions:

  • --static-swift-stdlib is enabled (it only affects "target" destination products, "host" destination products are always dynamic)
  • the building subset contains both "host" and "target" destination products derived from the same product.
  • the product imports Foundation (that has private dependency libs)

then the build randomly failed due to the race condition of the Objects.LinkFileList creation.

Reproducible project https://github.com/kateinoigakukun/swift-autolink-issue-repro

Modifications:

This commit fixes the issue by distinguishing the temporary link file list response file name by the -tool suffix.

I think it would be better to add assertions in LLBuildManifest to avoid such unintentional target overwrites later.

Result:

Fix the build for the above case.

@kateinoigakukun kateinoigakukun force-pushed the katei/fix-exe-prod-plugin-static branch from 18f5ebb to b2fd9a4 Compare June 21, 2024 17:08
@kateinoigakukun
Copy link
Member Author

@swift-ci test

@kateinoigakukun
Copy link
Member Author

@swift-ci test

@kateinoigakukun
Copy link
Member Author

@swift-ci test

@kateinoigakukun
Copy link
Member Author

@swift-ci test Windows

@kateinoigakukun kateinoigakukun marked this pull request as ready for review June 22, 2024 06:30
@kateinoigakukun
Copy link
Member Author

@swift-ci test

…dlib`

Given the following conditions:
- `--static-swift-stdlib` is enabled (it only affects "target" destination
  products, "host" destination products are always dynamic)
- the building subset contains both "host" and "target" destination products
  derived from the same product.
- the product imports `Foundation` (that has private dependency libs)

then the build randomly failed due to the race condition of the
Objects.LinkFileList creation.

This commit fixes the issue by distinguishing the temporary link file
list response file name by the `-tool` suffix.
This is usually the case when building with some of build-presets
without `build-swift-static-stdlib` option.
@kateinoigakukun kateinoigakukun force-pushed the katei/fix-exe-prod-plugin-static branch from 80bf108 to b7b1f8e Compare June 23, 2024 02:36
@kateinoigakukun
Copy link
Member Author

@swift-ci test

@kateinoigakukun
Copy link
Member Author

@swift-ci test Windows

2 similar comments
@kateinoigakukun
Copy link
Member Author

@swift-ci test Windows

@kateinoigakukun
Copy link
Member Author

@swift-ci test Windows

@MaxDesiatov MaxDesiatov requested a review from xedin June 23, 2024 15:34
Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Great catch, thank you!

@MaxDesiatov MaxDesiatov merged commit e858647 into swiftlang:main Jun 23, 2024
5 checks passed
kateinoigakukun added a commit that referenced this pull request Jun 24, 2024
…ce (#7698)

Add debug assertion to avoid llbuild command name conflicts.

Depends on #7695

### Motivation:

One of the reasons we couldn't reveal
#7695 issue for long
time (at least past 2 releases) is that we overlooked name conflicts on
`addWriteLinkFileListCommand`.

Looking into the manifest builder, some of the commands also forgot to
assert name conflicts, so it was error-prone.

### Modifications:

Add a convenience method to perform assertions consistently

### Result:

NFC for release build. Just add debug assertions
MaxDesiatov pushed a commit that referenced this pull request Jul 2, 2024
…dlib` (#7695)

### Motivation:

Given the following conditions:
- `--static-swift-stdlib` is enabled (it only affects "target"
destination products, "host" destination products are always dynamic)
- the building subset contains both "host" and "target" destination
products derived from the same product.
- the product imports `Foundation` (that has private dependency libs)

then the build randomly failed due to the race condition of the
Objects.LinkFileList creation.

Reproducible project
https://github.com/kateinoigakukun/swift-autolink-issue-repro

### Modifications:

This commit fixes the issue by distinguishing the temporary link file
list response file name by the `-tool` suffix.

I think it would be better to add assertions in `LLBuildManifest` to
avoid such unintentional target overwrites later.

### Result:

Fix the build for the above case.

(cherry picked from commit e858647)
MaxDesiatov added a commit that referenced this pull request Jul 2, 2024
Cherry-pick of #7695.

**Explanation**: Given the following conditions:
- `--static-swift-stdlib` is enabled (it only affects "target" destination products, "host" destination products are always dynamic)
- the building subset contains both "host" and "target" destination products derived from the same product.
- the product imports `Foundation` (that has private dependency libs)

then the build randomly failed due to the race condition of the `Objects.LinkFileList` creation.

Reproducible project: https://github.com/kateinoigakukun/swift-autolink-issue-repro

**Scope**: isolated to packages with plugins
**Risk**: low, the change was incubated on `main` for a few weeks now
**Testing**: Added a new automated test case 
**Issue**: N/A
**Reviewer**: @MaxDesiatov

Co-authored-by: Yuta Saito <[email protected]>
norio-nomura added a commit to norio-nomura/docker-swiftlint that referenced this pull request Jul 10, 2024
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.

3 participants