Skip to content

Conversation

owenv
Copy link
Contributor

@owenv owenv commented Sep 18, 2025

swiftlang/swift-llbuild#1013 removes swift-build-tool from the products of llbuild, as it's no longer used during SwiftPM builds or bootstrapping. Remove the reference in build-script-impl

@owenv
Copy link
Contributor Author

owenv commented Sep 18, 2025

@swift-ci test

@owenv
Copy link
Contributor Author

owenv commented Sep 18, 2025

@swift-ci build toolchain

@owenv
Copy link
Contributor Author

owenv commented Sep 18, 2025

It's possible this change is too narrow - however, we do still need to build llbuild here to support the SwiftPM bootstrap, we just don't need to install anything it produces

@owenv owenv requested a review from shahmishal September 18, 2025 00:11
Copy link
Member

@etcwilde etcwilde left a comment

Choose a reason for hiding this comment

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

If it's only used for bootstrapping, do we need llbuild in build-script-impl at all? It looks like SwiftPM's bootstrap script already builds llbuild, or at least has logic for building it:

def build_llbuild(args):
    """Builds LLBuild using CMake."""
    logging.info("Building llbuild")

    # Set where we are going to build llbuild for future steps to find it
    args.build_dirs["llbuild"] = os.path.join(args.target_dir, "llbuild")

    api_dir = os.path.join(args.build_dirs["llbuild"], ".cmake/api/v1/query")
    mkdir_p(api_dir)
    call(["touch", "codemodel-v2"], cwd=api_dir, verbose=args.verbose)

    flags = [
        "-DCMAKE_C_COMPILER:=%s" % (args.clang_path),
        "-DCMAKE_CXX_COMPILER:=%s" % (args.clangxx_path),
        "-DCMAKE_AR:PATH=%s" % (args.ar_path),
        "-DCMAKE_RANLIB:PATH=%s" % (args.ranlib_path),
        "-DLLBUILD_SUPPORT_BINDINGS:=Swift",
    ]
    cmake_env = []

    if platform.system() == 'Darwin':
        # On Darwin, make sure we're building for the host architecture.
        flags.append("-DCMAKE_OSX_ARCHITECTURES:=%s" % (get_build_target(args).split('-')[0]))
        # Inject linkage of C++ standard library
        cmake_env.append("LDFLAGS=-lc++")

    if args.sysroot:
        flags.append("-DSQLite3_INCLUDE_DIR=%s/usr/include" % args.sysroot)

    args.source_dirs["llbuild"] = get_llbuild_source_path(args)
    build_with_cmake(args, flags, [], args.source_dirs["llbuild"], args.build_dirs["llbuild"], cmake_env=cmake_env)

https://github.com/swiftlang/swift-package-manager/blob/a112ab817dafb40f3d95daaa31b034f7c766fa8e/Utilities/bootstrap#L663-L693

@owenv
Copy link
Contributor Author

owenv commented Sep 18, 2025

@etcwilde I considered that, but I wasn't sure what should be responsible for running the llbuild tests if I made that change.

@owenv owenv force-pushed the owenv/swift-build-tool branch from b835fb9 to f04ed6f Compare September 18, 2025 16:08
@owenv
Copy link
Contributor Author

owenv commented Sep 18, 2025

@swift-ci smoke test

@owenv
Copy link
Contributor Author

owenv commented Sep 18, 2025

@swift-ci build toolchain

@owenv
Copy link
Contributor Author

owenv commented Sep 18, 2025

@swift-ci please build toolchain windows

@owenv
Copy link
Contributor Author

owenv commented Sep 19, 2025

Unclear what the problem on Windows actually is

Archiving artifacts
‘build/artifacts/*’ doesn’t match anything, but ‘*’ does. Perhaps that’s what you mean?
ERROR: Step ‘Archive the artifacts’ failed: No artifacts found that match the file pattern "build/artifacts/*". Configuration error?

@finagolfin
Copy link
Member

Thanks for doing this, I've been doing the same with my native Android toolchain for a while now. Maybe dump a warning for people who are unknowingly using this flag still, and remove install-llbuild from all build presets, since it does nothing now?

I thought about removing the flag altogether, but it looks like it is automatically generated in build-script-impl, so that's not possible.

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