Skip to content

Add a build product and presets for swift-format. #32546

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

Merged
merged 3 commits into from
Jul 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions utils/build-presets.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1701,6 +1701,22 @@ assertions
swiftsyntax
swiftsyntax-verify-generated-files

#===------------------------------------------------------------------------===#
# Test Swift Format
#===------------------------------------------------------------------------===#

[preset: buildbot_swiftformat_macos]
mixin-preset=mixin_swiftpm_package_macos_platform
release
assertions
swiftformat

[preset: buildbot_swiftformat_linux]
mixin-preset=mixin_swiftpm_package_linux_platform
release
assertions
swiftformat

#===------------------------------------------------------------------------===#
# Test Swift Stress Tester
#===------------------------------------------------------------------------===#
Expand Down
2 changes: 2 additions & 0 deletions utils/build-script
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,8 @@ class BuildScriptInvocation(object):
product_classes.append(products.SwiftSyntax)
if self.args.build_skstresstester:
product_classes.append(products.SKStressTester)
if self.args.build_swiftformat:
product_classes.append(products.SwiftFormat)
if self.args.build_swiftevolve:
product_classes.append(products.SwiftEvolve)
if self.args.build_indexstoredb:
Expand Down
6 changes: 6 additions & 0 deletions utils/build_swift/build_swift/driver_arguments.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ def _apply_default_arguments(args):
args.test_indexstoredb = False
args.test_sourcekitlsp = False
args.test_skstresstester = False
args.test_swiftformat = False
args.test_swiftevolve = False
args.test_toolchainbenchmarks = False

Expand Down Expand Up @@ -571,6 +572,9 @@ def create_argument_parser():
option(['--skstresstester'], store_true('build_skstresstester'),
help='build the SourceKit stress tester')

option(['--swiftformat'], store_true('build_swiftformat'),
help='build swift-format')

option(['--swiftevolve'], store_true('build_swiftevolve'),
help='build the swift-evolve tool')

Expand Down Expand Up @@ -1020,6 +1024,8 @@ def create_argument_parser():
help='skip testing PlaygroundSupport')
option('--skip-test-skstresstester', toggle_false('test_skstresstester'),
help='skip testing the SourceKit Stress tester')
option('--skip-test-swiftformat', toggle_false('test_swiftformat'),
help='skip testing swift-format')
option('--skip-test-swiftevolve', toggle_false('test_swiftevolve'),
help='skip testing SwiftEvolve')
option('--skip-test-toolchain-benchmarks',
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions utils/build_swift/tests/expected_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
'build_tensorflow_swift_apis': False,
'build_libparser_only': False,
'build_skstresstester': False,
'build_swiftformat': False,
'build_swiftevolve': False,
'build_indexstoredb': False,
'test_indexstoredb_sanitize_all': False,
Expand Down Expand Up @@ -234,6 +235,7 @@
'test_indexstoredb': False,
'test_sourcekitlsp': False,
'test_skstresstester': False,
'test_swiftformat': False,
'test_swiftevolve': False,
'test_toolchainbenchmarks': False,
'tvos': False,
Expand Down Expand Up @@ -476,6 +478,7 @@ class BuildScriptImplOption(_BaseOption):
SetTrueOption('--swiftsyntax', dest='build_swiftsyntax'),
SetTrueOption('--build-libparser-only', dest='build_libparser_only'),
SetTrueOption('--skstresstester', dest='build_skstresstester'),
SetTrueOption('--swiftformat', dest='build_swiftformat'),
SetTrueOption('--swiftevolve', dest='build_swiftevolve'),
SetTrueOption('-B', dest='benchmark'),
SetTrueOption('-S', dest='skip_build'),
Expand Down Expand Up @@ -592,6 +595,7 @@ class BuildScriptImplOption(_BaseOption):
DisableOption('--skip-test-indexstore-db', dest='test_indexstoredb'),
DisableOption('--skip-test-sourcekit-lsp', dest='test_sourcekitlsp'),
DisableOption('--skip-test-skstresstester', dest='test_skstresstester'),
DisableOption('--skip-test-swiftformat', dest='test_swiftformat'),
DisableOption('--skip-test-swiftevolve', dest='test_swiftevolve'),
DisableOption('--skip-test-toolchain-benchmarks',
dest='test_toolchainbenchmarks'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from .sourcekitlsp import SourceKitLSP
from .swift import Swift
from .swiftevolve import SwiftEvolve
from .swiftformat import SwiftFormat
from .swiftinspect import SwiftInspect
from .swiftpm import SwiftPM
from .swiftsyntax import SwiftSyntax
Expand All @@ -48,6 +49,7 @@
'PlaygroundSupport',
'PythonKit',
'Swift',
'SwiftFormat',
'SwiftInspect',
'SwiftPM',
'TensorFlowSwiftAPIs',
Expand Down
100 changes: 100 additions & 0 deletions utils/swift_build_support/swift_build_support/products/swiftformat.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
# swift_build_support/products/swiftformat.py -----------------*- python -*-
#
# This source file is part of the Swift.org open source project
#
# Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
# Licensed under Apache License v2.0 with Runtime Library Exception
#
# See https://swift.org/LICENSE.txt for license information
# See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
#
# ----------------------------------------------------------------------------

import os

from build_swift.build_swift.constants import MULTIROOT_DATA_FILE_PATH

from . import cmark
from . import foundation
from . import libcxx
from . import libdispatch
from . import libicu
from . import llbuild
from . import llvm
from . import product
from . import swift
from . import swiftpm
from . import swiftsyntax
from . import xctest
from .. import shell


class SwiftFormat(product.Product):
@classmethod
def product_source_name(cls):
"""product_source_name() -> str
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a python expert - are doc strings inherited from super classes? If so, I would drop this duplicate copy of the doc string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are doc strings inherited from super classes?

It depends on the version of Python you're using. Inheritance for docstrings was added in Python 3 (I think 3.5, but can't recall or find a reference quickly). I believe this project uses Python 2.X so doc strings aren't inherited in this context.

For additional context, note that the other Product subclasses duplicate this doc string as well.

Let me know if you would prefer that I delete the doc string or otherwise refer to the superclass' doc string. If so, you may want to apply the same to the other Product subclasses.

Copy link
Contributor

Choose a reason for hiding this comment

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

note that the other Product subclasses duplicate this doc string as well.

Yeah, it looks like we're inconsistent about this in two different ways:

  • Not all products duplicate doc strings
  • For products that do, they are not duplicating the doc strings for all methods, just some (e.g. this one).

Anyway, I won't hold up the patch over this - we can clean it up separately across all products.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree there's inconsistency here. I'm fine with whatever approach you prefer. I don't write Python very often so I can't claim to know what's the best practice here.


The name of the source code directory of this product.
"""
return "swift-format"

@classmethod
def is_build_script_impl_product(cls):
return False

@classmethod
def is_swiftpm_unified_build_product(cls):
return True

def run_build_script_helper(self, action, additional_params=[]):
script_path = os.path.join(
self.source_dir, 'build-script-helper.py')

configuration = 'release' if self.is_release() else 'debug'

helper_cmd = [
script_path,
action,
'--toolchain', self.install_toolchain_path(),
'--configuration', configuration,
'--build-path', self.build_dir,
'--multiroot-data-file', MULTIROOT_DATA_FILE_PATH,
# There might have been a Package.resolved created by other builds
# or by the package being opened using Xcode. Discard that and
# reset the dependencies to be local.
'--update'
Copy link
Contributor

Choose a reason for hiding this comment

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

--update seems suspect to run during testing when it is expected to use local sources. @nathawes I see this is copied from the stress tester - can you explain when there would be a package.resolved created? Opening it in Xcode in particular would create a resolved file in a location used by Xcode, not where it is for command-line builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure off of the top of my head, but we tell contributors they can invoke build-script-helper themselves passing a swift.org toolchain path in addition to using it to to get swiftpm to generate an Xcode project the old way (we add additional search paths via an .xcconfig file), so my guess would be one of those does it. Looks like Alex added this in #28005. @ahoppen is that what was creating the problematic package.resolved here?

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, I'm okay with taking this change without resolving this issue, since it's evidently not causing problems for the stress tester. But we should figure it out.

Copy link
Member

Choose a reason for hiding this comment

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

The issue is what’s described in the comment above it 😉. Maybe a little more context.

  1. You build swift-format using this build script, Package.resolved gets set to the local dependencies.
  2. You open the project in Xcode. SWIFTCI_USE_LOCAL_DEPS is not set and thus Package.resolved gets reset to use remote dependencies.
  3. You run a build using this script again. Without the --update option (which again re-generates Package.resolved), we are now trying to create a unified build of a local SwiftSyntax and a swift-format that takes a remote dependency.

I can’t recall the exact error scenario, but possible options include:

  • SwiftPM isn’t happy if the versions of SwiftSyntax don’t refer to the same source code
  • I wanted to make sure that we are definitely using the local dependency and are never stuck on a remote dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, Xcode will use its own Package.resolved that would not interfere with the local build. However, I suppose if you built from command-line using swift build it would do the same thing you are describing.

Copy link
Member

Choose a reason for hiding this comment

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

@benlangmuir Does it? I’m fairly sure, Xcode uses the same Package.resolved. Otherwise there also wouldn’t be any point in checking Package.resolved into source control (for other projects).

]
if self.args.verbose_build:
helper_cmd.append('--verbose')
helper_cmd.extend(additional_params)

shell.call(helper_cmd)

def should_build(self, host_target):
return True

def build(self, host_target):
self.run_build_script_helper('build')

def should_test(self, host_target):
return self.args.test_swiftformat

def test(self, host_target):
self.run_build_script_helper('test')

def should_install(self, host_target):
return False

@classmethod
def get_dependencies(cls):
return [cmark.CMark,
llvm.LLVM,
libcxx.LibCXX,
libicu.LibICU,
swift.Swift,
libdispatch.LibDispatch,
foundation.Foundation,
xctest.XCTest,
llbuild.LLBuild,
swiftpm.SwiftPM,
swiftsyntax.SwiftSyntax]