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

[SR-1393] [SwiftPM] Enforce Swift module import dependencies #5297

Open
ddunbar opened this issue May 3, 2016 · 16 comments
Open

[SR-1393] [SwiftPM] Enforce Swift module import dependencies #5297

ddunbar opened this issue May 3, 2016 · 16 comments
Labels

Comments

@ddunbar
Copy link
Member

@ddunbar ddunbar commented May 3, 2016

Previous ID SR-1393
Radar rdar://problem/23237241
Original Reporter @ddunbar
Type Bug
Status In Progress
Resolution
Additional Detail from JIRA
Votes 9
Component/s Package Manager
Labels Bug
Assignee owenvoorhees (JIRA)
Priority Medium

md5: b673faead2f75706db0bdac53b7b8ef3

is duplicated by:

  • SR-4539 Build executable with modules, without specified dependencies
  • SR-8344 SPM include path is too permissive, leads to link errors that should have been compile errors
  • SR-9359 SwiftPM should have better diagnostics if tests depend on executable target
  • SR-11758 Undefined symbol: static allcases.MyEnum.allCases.getter : [allcases.MyEnum]

relates to:

  • SR-3033 Poor diagnostic when importing executable target module in a test
  • SR-13651 Linker error while linking unit test cases
  • SR-3205 Ability to restrict the import of implicit module dependencies
  • SR-14212 Removing package from Package.swift doesn't result in import error
  • SR-7483 Deserialization Failure

Issue Description:

Currently, SwiftPM passes a -I to the directory containing all built modules, which allows any Swift code to import any module which has been built.

This is unfortunate, as it means code can easily import modules which haven't been explicitly declared as a dependency. That might frequently work but it can be the source of transient build failures or "works for me" type problems.

Instead, we should enforce that Swift code is only allowed to import modules which have been explicitly declared as dependencies. We could also let this include transitive dependencies, but I would actually prefer we require any module explicitly declare all other modules it is allowed to import.

swiftc doesn't expose a mechanism currently we can use to implement this directly. We could:

  • Extend swiftc to explicitly support this.

  • Build symlink trees for each target we want to build, and point the -I at that instead, to enforce what is allowed.

@ddunbar
Copy link
Member Author

@ddunbar ddunbar commented May 3, 2016

/cc @belkadan any preference on the implementation approach here? For the swiftc route, I was imagining some kind of manifest file we could hand to swiftc which described exactly which modules could or could not be imported. This would allow us to give substantially better diagnostics, and could play into a long term story for automatically assisting with dependency management.

@aciidb0mb3r
Copy link
Member

@aciidb0mb3r aciidb0mb3r commented May 3, 2016

We already create build dir for each module, what about creating the swiftmodules in those directories and -I to them instead of entire debug/release folder?

@ddunbar
Copy link
Member Author

@ddunbar ddunbar commented May 3, 2016

We could do that too... I would rather not end up with a massive amount of -I arguments. Also, I think we probably need to tread carefully here with how those modules find the other modules they depend on... it may be the case that my proposed "symlink tree" approach doesn't actually work for this reason.

@belkadan
Copy link

@belkadan belkadan commented May 3, 2016

I don't like the idea of repeating information, but I guess we already have that (since the imported module name isn't enough to know where to get the thing). How does this work with packages containing multiple modules, though?

@aciidb0mb3r
Copy link
Member

@aciidb0mb3r aciidb0mb3r commented May 3, 2016

it works something like this currently:
Internal modules can be imported without declaring them as dependencies but it might result in build failures

  1. The module which is imported is not built when the importing module is building.
  2. The importing module is a product and it'll produce linker error.

External dependencies are implicit dependencies to all of internal modules so they can always be imported which I think is correct behavior.

@belkadan
Copy link

@belkadan belkadan commented May 3, 2016

Sorry, I meant "what's the plan for packages containing multiple modules?". Do we want to worry about "you didn't declare a dependency on this module, but someone else did, and you do depend on something else in the same package"?

@swift-ci
Copy link
Contributor

@swift-ci swift-ci commented Jul 23, 2018

Comment by Shawn Morel (JIRA)

@aciidb0mb3r Going to bump the discussion here since this bug is more than 2 years old and you just closed a bug I filed as a duplicate. I think the usability of SPM is a place I might be able to get might feet wet 🙂

Reading through the comments here, I see some overlap with what I mentioned in [SR-8344] as possible approaches but 1) the state of the toolchain has evolved since may 2016 so no everything still applies and 2) it's not 100% clear to me where along the spm, sbt, swiftc chain the fix belongs.

I think the easiest / quickest fix would be to pass multiple -I import search paths and not keep the .swiftmodules at the top of the build artifacts tree. I don't know how this overlaps with @belkadan's latest proposal for .swiftinterface files and using .swiftmodules as a compiler cache mechanism only.

This also doesn't address the issue of the dependencies implicitly emitted with the '-emit-dependencies' flag and the .o files being directly listed in the Objects.LinkedFileList

From a usability standpoint, I think it's poor behaviour if something compiles only to fail with a linker error about a mangled symbol. It also impedes building tools like a well behaved language server for VScode on linux where explicit search paths can be queried easily.

Any advice on direction is appreciated

@aciidb0mb3r
Copy link
Member

@aciidb0mb3r aciidb0mb3r commented Jul 23, 2018

@belkadan do you have any advice on how to implement this? What if SwiftPM passed the list of module names that are allowed to be imported?

@belkadan
Copy link

@belkadan belkadan commented Jul 23, 2018

If you're willing to allow transitive dependencies, multiple search paths seems like a very composable answer. If you want to catch accidental uses of transitive dependencies, you'd need something more like the "allow" list, but I don't know how you'd exempt system modules from that checking.

@aciidb0mb3r
Copy link
Member

@aciidb0mb3r aciidb0mb3r commented Jul 23, 2018

I discussed this with Jordan. We think creating a response file per-module with list of all search paths it depends on is a reasonable solution. There will be some performance impact as the compiler will need to do extra work but it shouldn't be significant.

@ddunbar
Copy link
Member Author

@ddunbar ddunbar commented Jul 24, 2018

@aciidb0mb3r just so I'm clear, meaning we will do work in SwiftPM to just put all the modules in separate places and pass a long list of them? Or this is depending on some new feature of the Swift compiler.

I'm personally fine trying to solve this in the PM exclusively to start with, since it will involve less moving parts even if it does make for gross command lines. We could also use symlink trees to avoid the gross command lines.

@aciidb0mb3r
Copy link
Member

@aciidb0mb3r aciidb0mb3r commented Jul 24, 2018

Right, we will put module files in target specific directories and add include paths. Command line won't be gross because the compiler can accept response files now. This will be similar to the linked filelist we pass to the compiler.

@weissi
Copy link
Member

@weissi weissi commented Mar 30, 2020

We still have terrible diagnostics for this. @aciidb0mb3r is the quickest way to get good diagnostics for the common case (https://bugs.swift.org/browse/SR-9359) fixing this issue? If yes, then we should really prioritise it; if no and there's a quicker win, we should probably un-dupe https://bugs.swift.org/browse/SR-9359 .

So many people run into the 'tests depending on executable target' problem, I can't count the numbers I've personally wasted on this or the number of times I've told others.

@abertelrud
Copy link
Contributor

@abertelrud abertelrud commented Mar 30, 2020

Since the diagnostics are coming from underlying tools, I think that separating out the sets of module files so you can't import the module for a component you can't link against seems like the best way to fix it. As I understand it, that's what this bug report covers. I agree that this should be prioritized.

@weissi
Copy link
Member

@weissi weissi commented Apr 29, 2021

@tomerd this is still an issue today and causes failures that are hard to debug for anybody who isn't super experienced with SwiftPM and knows that any "missing module XYZ" likely means that some Package.swift file somewhere in the graph has a missing dependency. Even when knowing that fact, it can still be incredibly tedious to track down.

@swift-ci
Copy link
Contributor

@swift-ci swift-ci commented May 6, 2021

Comment by David Evans (JIRA)

The most frustrating issue is the non-determinism. It can (and has) cause a lot of confusion when builds can fail/succeed one after another without source changes.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants