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 bug that causes ignored 'exclude' directive #83

Merged
merged 1 commit into from
Dec 21, 2015
Merged

Fix bug that causes ignored 'exclude' directive #83

merged 1 commit into from
Dec 21, 2015

Conversation

bppr
Copy link
Contributor

@bppr bppr commented Dec 17, 2015

When building a Package that has one or more Targets specified,
the exclude gets treated as though it's a key of the latest target.
By moving the exclude directive above Target, this bug is fixed.

@kostiakoval
Copy link
Contributor

Great that you have fount that bug. I think it would better to keep a 29_exclude_directory test as it is with no targets and add a new one. That way you could guaranty that both case would work.

//`29_exclude_directory
let package = Package(
     name: "29_exclude_directory",
     exclude: ["FooLib"]

//`30_exclude_directory_with_target
let package = Package(
     name: "29_exclude_directory",
     targets: [Target(name: "FooLib")],
     exclude: ["FooLib"]

@bppr
Copy link
Contributor Author

bppr commented Dec 17, 2015

Got it. Updated bppr/master with a separate functional test.

@kostiakoval
Copy link
Contributor

The TOML format is basically a hash table with Keys and Values. The order of the operators (exclude or targets first) shouldn't have an impact on the output.
If I move this code to line 116, the test wont pass.

result += "\n" + "exclude = \(exclude)" + "\n"

As other good example switching order for testDependencies and dependencies doesn't have any impact on the app.
It seams (at least for me) that there is an error in TOML Formatting.

@bppr
Copy link
Contributor Author

bppr commented Dec 17, 2015

The order of the operators (exclude or targets first) shouldn't have an impact on the output

Agreed, it shouldn't -- but it does. I'm not super-familiar with TOML, so I don't know if there's a good way to tell TOML that the key you're adding is not part of the key/value set for a Target, but what ends up happening is that the table for the last target ends up with an exclude key, and not the root Package itself.

[package]
name = "whatevs"
# more TOML here

[[package.targets]]
name = "TargetOne"

[[package.targets]]
name = "TargetTwo"

exclude = ["TargetTwo"]

If that's done, TOML thinks exclude is part of what's specified for "TargetTwo" -- that's the issue that needs to be fixed -- the easiest fix is just to move exclude up so it's not confused with any keys associated with Target, but if we want to dive in and do something else to fix this, I'm all ears.

@bppr
Copy link
Contributor Author

bppr commented Dec 17, 2015

Also, I've just noticed that if I change the order such that it writes targets first, before dependencies or testDependencies, it does break. In fact, it fails to even bootstrap itself properly. So things, as they stand, are order-dependent in that we can't put anything that is intended to be a key for the top-level package after the target declarations.

@mxcl
Copy link
Contributor

mxcl commented Dec 18, 2015

Yeah the exclude needs to be parented with the [package] part.

@mxcl
Copy link
Contributor

mxcl commented Dec 18, 2015

Thanks for this, would be good to add a comment to the new test linking here for reference.

@bppr
Copy link
Contributor Author

bppr commented Dec 18, 2015

Will do.

When building a `Package` that has one or more `Target`s specified,
the `exclude` gets treated as though it's a key of the latest target.
By moving the `exclude` directive above `Target`, this bug is fixed.
@bppr
Copy link
Contributor Author

bppr commented Dec 18, 2015

How's this, @mxcl?

mxcl added a commit that referenced this pull request Dec 21, 2015
Fix bug that causes ignored 'exclude' directive
@mxcl mxcl merged commit 828d1e0 into swiftlang:master Dec 21, 2015
aciidgh pushed a commit to aciidgh/swift-package-manager that referenced this pull request Jan 11, 2019
Fix MSVC warnings for unknown escape character sequence
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.

3 participants