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

Scheme config variants wrong assignment for similar config names #976

Merged
merged 12 commits into from
May 1, 2021

Conversation

stefanomondino
Copy link
Contributor

This fixes #975

To determine proper match between a config and a config variant, we should strip away the Release/Debug word from config name and check if the remaining part (trimmed out of whitespaces) is equal to the variant name.

Also, while investigating this and implementing proper tests, I found out a bug in usual tests with a mismatch between a Prod config variant and related Debug Production and Release Production, which is an error as per documentation. Tests were working only because Debug Production contains Prod, but it was just a coincidence.

This is why the TestProject.swift file was changed.

Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Thanks for this @stefanomondino. Sorry for the delay on the review.

I think I can see a potential breaking change here if someone had a config name of Production debug for example, as the replacement you're using is case sensitive. Some people also don't put debug or release in their configs, and might have Appstore for example. This would be good test cases too.
Perhaps another approach would be to see if the whole word is in the config? Splitting by spaces, and then checking if that contains the variant?

Could you also please add a changelog entry?

@stefanomondino
Copy link
Contributor Author

Hey @yonaskolb,

Thanks for the review (and for the huge amount of time you're saving me with this project <3 )

I'm not quite sure what you mean here, please correct me if I'm wrong.
debug and release are not part of the config name, they are the type of each config (should be some kind of default template provided by xcode itself).
When using config variants, we're actually autogenerating schemes that follow a convention (the debug template of selected configuration for Run and Test, and the release one for archive).
If someone were using a direct config name like Production debug it would probably end up having two autogenerated variants, Production debug debug and Production debug release, which is... strange.

Am I missing something?

Config(name: "Test Release", type: .release),
Config(name: "Production Release", type: .release),
Config(name: "PREPROD Release", type: .release),
Copy link
Owner

Choose a reason for hiding this comment

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

I mean if you were to change this to Prepod release I think you'd get a crash as the config can't be found with the variant name

@yonaskolb
Copy link
Owner

Glad you’re getting some good use out of it! 😀
See my comment above. I also realised this lookup logic is duplicated in the spec validation, so it should be pulled out into somewhere common

if !configs.contains(where: { $0.name.contains(configVariant) && $0.type == .debug }) {

@stefanomondino
Copy link
Contributor Author

Hey @yonaskolb I've pushed some changes as requested (extension over collection of Config objects) and updated the changelog, but still not getting your point about uppercase/lowercase... is this working for you now?

Config(name: "Test Release", type: .release),
Config(name: "Production Release", type: .release),
Config(name: "PreProd Release", type: .release),
Copy link
Owner

Choose a reason for hiding this comment

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

I think this will fail the tests, and I'm sure it's not uncommon to have that lowercase

Suggested change
Config(name: "PreProd Release", type: .release),
Config(name: "PreProd release", type: .release),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yonaskolb took me a while to get the point, then it totally made sense!

Should be fixed now as you originally suggested: whitespace tokenization and lowercase compare

Sources/ProjectSpec/Config.swift Outdated Show resolved Hide resolved
Config(name: "Test Release", type: .release),
Config(name: "Production Release", type: .release),
Config(name: "PreProd Release", type: .release),
Config(name: "Prod Release", type: .release),
Copy link
Owner

Choose a reason for hiding this comment

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

Could we make 2 of these use lowercase debug and release so it tests what we talked about? Maybe PreProd debug and PrePod release

Sources/ProjectSpec/Config.swift Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
stefanomondino and others added 3 commits April 12, 2021 12:57
Co-authored-by: Yonas Kolb <[email protected]>
- duplicated test for config variant name (uppercase/lowercase)
@@ -49,6 +49,9 @@

[Commits](https://github.com/yonaskolb/XcodeGen/compare/2.18.0...2.19.0)

#### Fixed
- Lookup scheme config variants by whole words, fixing incorrect assignment in names that contain subtrings of each other (eg PreProd and Prod) [#976](https://github.com/yonaskolb/XcodeGen/pull/976) @stefanomondino
Copy link
Owner

Choose a reason for hiding this comment

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

I'll move this on master

@yonaskolb yonaskolb merged commit dfe7f28 into yonaskolb:master May 1, 2021
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.

Scheme config variants wrong assignment for similar config names
2 participants