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.

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

Fix missing external target settings with config conditions #6170

Merged
merged 4 commits into from Apr 16, 2024

Conversation

fortmarek
Copy link
Member

Resolves #6157

Short description 📝

The issue with the Lookin dependency is that it has a .define setting with a config condition: https://github.com/QMUI/LookinServer/blob/develop/Package.swift#L53

While we decode that condition, we'd never actually use that setting.

The solution is to add the .define to the specified configuration settings.

How to test the changes locally 🧐

  • Generate app_with_spm_dependencies
  • Ensure SPM_LOOKIN_SERVER_ENABLED is defined for Debug configuration
  • Install the lookin app and run the fixture app -> you should see the app in the Lookin app

Contributor checklist ✅

  • The code has been linted using run mise run lint:fix
  • The change is tested via unit testing or acceptance testing, or both
  • The title of the PR is formulated in a way that is usable as a changelog entry
  • In case the PR introduces changes that affect users, the documentation has been updated

Reviewer checklist ✅

  • The code architecture and patterns are consistent with the rest of the codebase
  • Reviewer has checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed, and the title is usable as a changelog entry

@fortmarek fortmarek added the changelog:fixed PR will be listed in the Fixed section of CHANGELOG label Apr 10, 2024
@fortmarek fortmarek requested a review from pepicrft April 10, 2024 14:38
}

func settingsForPlatforms(_ platforms: [PackageInfo.Platform]) throws -> TuistGraph.SettingsDictionary {
var resolvedSettings = try settingsDictionaryForPlatform(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to get a settings dictionary for a platform nil? I'd iterate on the implementation to make the code more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's nil, it doesn't take a platform in the account. It was here before, but sure, let me refactor it 👍

@topkim993
Copy link

topkim993 commented Apr 10, 2024

I want to test it, but how can I tuist install and tuist generate locally?

Due to mise, it runs as 4.9.0.

mise tuist@4.9.0 ✓ installed

If there are any related documents, can I provide guidance?

I confirmed that it was processed automatically.

via mise ⚠️ pnpm is a community-developed plugin

@fortmarek
Copy link
Member Author

@topkim993 The easiest way is to usually run tuist via Xcode as described here

@rakuyoMo
Copy link

@fortmarek @pepicrft Hello, Is there any new development here? I hope the next version will contain this fix, it's really important to us.

@fortmarek fortmarek merged commit 88a1763 into main Apr 16, 2024
8 checks passed
@fortmarek fortmarek deleted the fix/dependencies-config-condition branch April 16, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:fixed PR will be listed in the Fixed section of CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LookinServer cannot be operated through Tuist Dependency
4 participants