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

dependencies are not strict #344

Open
mpalczew opened this issue Oct 25, 2019 · 15 comments
Open

dependencies are not strict #344

mpalczew opened this issue Oct 25, 2019 · 15 comments
Labels
P2 We should fix this this quarter

Comments

@mpalczew
Copy link

mpalczew commented Oct 25, 2019

A.swift

public class A {
    public init() {}
}

B.swift

import A

public class B {
    let a: A = .init()
    public init() {}
}

C.swift

import B // Should be an error B is not in transitive closure of deps

class C {
    let b: B = .init()
}

BUILD

load("@build_bazel_rules_swift//swift:swift.bzl", "swift_library")

swift_library(
    name = "A",
    srcs = ["A.swift"],
)

swift_library(
    name = "B",
    srcs = ["B.swift"],
    deps = [":A"],
)

swift_library(
    name = "C",
    srcs = ["C.swift"],
    deps = [":A"],
)
bazel clean
bazel build :C  # fails
bazel build :B 
bazel build :C # succeeds should fail
@kastiglione
Copy link
Contributor

fwiw a workaround is to have one library per package/directory.

@keith
Copy link
Member

keith commented Oct 25, 2019

To verify, this fails even with sand boxing enabled?

@thomasvl
Copy link
Member

Also, exactly what flags do you have enabled on the build? Isn't the a feature for a side module cache, do you have that enabled and thus you might have enabled this directly?

@allevato
Copy link
Member

Yeah, as @keith mentioned, this sounds like you're building without sandboxing enabled. All libraries in the same package emit their .swiftmodules to the same bazel-bin subdirectory, and that subdirectory will be passed to swiftc as a search path, so the second build of :C will see B.swiftmodule even though it's not an input to the compile action.

We could make this less likely be emitting each .swiftmodule into its own target-specific subdirectory, but that would increase the overall number of search paths passed to the compiler, which would increase the number of unnecessary filesystem accesses that the compiler makes to find modules. So there would be trade-offs, and likely a performance regression.

This isn't a Swift-specific issue; as another example, without sandboxing, an Objective-C source file could import headers from anywhere in the workspace, including headers that aren't in the deps of the target being compiled.

@kastiglione
Copy link
Contributor

but that would increase the overall number of search paths passed to the compiler

Correct me if I'm wrong, but this is only in cases of more than one swift_library per package. Otherwise, I believe the number of search paths would by the same if there's a 1:1 library per package.

@allevato
Copy link
Member

Correct me if I'm wrong, but this is only in cases of more than one swift_library per package.

Right, which is the case the user has above, and would be the general case since there is no requirement that a package only contains a single swift_library.

To flip it around, if you only have one swift_library per package, you wouldn't notice the performance regression because you've already done it to yourself. 🙂

@kastiglione
Copy link
Contributor

You're not getting me that easily, the swift compiler did it to me.

@kastiglione
Copy link
Contributor

I'll say this though, our experience with bazel is that, time after time, correctness gets chosen over performance. I say this not to advocate one thing or another, but just to gripe 😄

@mpalczew
Copy link
Author

mpalczew commented Oct 25, 2019

To verify, this fails even with sand boxing enabled?

I believe so. How do I make sure sandboxing is enabled?

My bazelrc

build --apple_platform_type=ios

Also tried with and without

build --strategy=SwiftCompile=standalone

Same result.

It does bother me that the output of the build results appears to be non-deterministic.

e.g. bazel build ... or bazel test ...
may or may not fail depending on what ends up getting executed first which may depend on remote cachine and how many cores are working at a time. i.e. might work on a 1 core machine, and fail on a 16 core machine.

@allevato
Copy link
Member

You can try --strategy=SwiftCompile=sandboxed to force it to use the sandbox, to see if that prints out any messages about the sandbox not being enabled somehow.

...which makes me wonder now, since SwiftCompile also supports workers for incremental compilation, the default strategy for that action is actually remote,worker,sandboxed,local, meaning that the worker will be preferred over sandboxed execution. And worker execution is specifically not sandboxed (the default value for --worker_sandboxing is false) so that it can non-hermetically store incremental state outside of the regular output locations in order to provide that incrementality. So that might actually be what's going on here.

What's not clear is how to resolve it; there's a --sandbox_writable_path flag that you can use to pass additional paths that the sandbox should let you write into, but the path where the incremental state is stored is also under blaze-out/<configuration>/bin/_swift_incremental, so it's not something that can be easily determined a priori to pass on the command line (or add to .bazelrc).

So for now, I think you'll have to explicitly force the sandboxed strategy, which means you'll lose swiftc -incremental mode, which may or may not matter to you. If you're strictly concerned about determinism, then you probably don't want incremental mode, since the Swift compiler does write timestamps into the .swiftdeps files that it generates.

@mpalczew
Copy link
Author

Just to circle back here and provide more context.

Sandboxed does build what I need it to build and behaves appropriately.
However due to bugs like #310
And using the workaround, sandboxed is the only way I can build.

Otherwise it will add duplicate modules to the path in some circumstances.

@mpalczew
Copy link
Author

ok, found another problem.

When using remote caching, and compiling non sandboxed, and then compiling sandboxed. The nonsandboxed builds fine, gets cached and then the sandboxed build will use the version that was cached, which really should have never compiled.

@tinder-maxwellelliott
Copy link

Just to circle back here and provide more context.

Sandboxed does build what I need it to build and behaves appropriately.
However due to bugs like #310
And using the workaround, sandboxed is the only way I can build.

Otherwise it will add duplicate modules to the path in some circumstances.

I am also experiencing situations where I am getting duplicate modules, did adding --strategy=SwiftCompile=sandboxed stop this from happening to you?

@keith keith added the P2 We should fix this this quarter label Oct 26, 2021
@keith
Copy link
Member

keith commented Oct 26, 2021

On the original issue here I think the way out here is to support the new explicit module compilation mode, which will likely eliminate the pollution and the duplicate module issue

@keith
Copy link
Member

keith commented May 16, 2024

FYI you can use --features=swift.use_explicit_swift_module_map to avoid this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We should fix this this quarter
Projects
None yet
Development

No branches or pull requests

6 participants