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

dylansturg · 2020-06-25T17:20:08Z

There are 2 related changes here:

A new build Product that builds and tests swift-format by calling its helper script, added in swift-format PR 207.
Create a macOS and Linux preset that builds and tests swift-format on those platforms.

These changes allow building & testing swift-format as part of the Swift project, so that swift-format can be added to the Swift project's CI bots.

New Flags

There are new command line options to support the swift-format product:

--swiftformat: Enables building swift-format.
--skip-test-swiftformat: Disables running tests for swift-format after building.

Installing

Installing is intentionally not implemented because swift-format isn't ready to be installed as part of the Swift toolchain.

Background

This PR implements steps 5 and 6 of the instructions to get swift-format ready to be built using Swift's CI bots:
https://forums.swift.org/t/testing-swift-format-as-part-of-swift-s-ci-infrastructure/30619

allevato · 2020-06-25T17:22:29Z

cc @benlangmuir @akyrtzi

gottesmm

Just a quick drive by I noticed when reading the commit stream. Don't treat this as a true review.

gottesmm · 2020-06-25T17:31:21Z

utils/build_swift/build_swift/driver_arguments.py

@@ -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 the SourceKit stress tester')


The help comments here and below need to be updated.

Oops. Thanks!

This product supports building and testing swift-format, forwarding the commands to a build script inside of swift-format's repo to handle actually invoking Swift PM. There are new command line options to support the swift-format product: `--swiftformat`: Enables building swift-format. `--skip-test-swiftformat': Disables running tests for swift-format after building. Installing is intentionally not implemented because swift-format isn't ready to be installed as part of the Swift toolchain.

benlangmuir · 2020-06-25T20:55:35Z

utils/build-presets.ini

+release
+assertions
+swiftsyntax
+swiftsyntax-verify-generated-files


Does --swiftsyntax run tests? If so, these new presets should skip them.

@akyrtzi is --swiftsyntax-verify-generated-files relevant for swift-format, or just for swift syntax itself?

I think this is only useful for swift / swift-syntax testing. (/cc @ahoppen).

The comments in this file suggest this is used to verify the checked-in generated files are correct, since they won't be generated. I guess it's safe to assume that the checked-in generated files are safe to use in this context without verifying them?

I guess it's safe to assume that the checked-in generated files are safe to use in this context without verifying them?

I believe so.

Sounds good. I have deleted this step, and added skip-test-swiftsyntax to also skip running swiftsyntax tests.

benlangmuir · 2020-06-25T20:59:17Z

utils/swift_build_support/swift_build_support/products/swiftformat.py

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


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.

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.

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.

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.

benlangmuir · 2020-06-25T21:05:52Z

utils/swift_build_support/swift_build_support/products/swiftformat.py

+            # 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'


--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.

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?

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.

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

You build swift-format using this build script, Package.resolved gets set to the local dependencies.

You open the project in Xcode. SWIFTCI_USE_LOCAL_DEPS is not set and thus Package.resolved gets reset to use remote dependencies.

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.

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.

@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).

utils/swift_build_support/swift_build_support/products/swiftformat.py

- Remove unused `package_name` method. - Disable swift-syntax tests in swift-format builder preset.

benlangmuir · 2020-06-26T16:33:57Z

@swift-ci please test

benlangmuir · 2020-06-26T16:35:42Z

@swift-ci please test macOS

swift-ci · 2020-06-26T18:19:53Z

Build failed
Swift Test OS X Platform
Git Sha - e56ce8a

benlangmuir · 2020-06-26T18:37:32Z

Please take a look at the python failures:
https://ci.swift.org/job/swift-PR-osx/21874/console

11:19:16 Failing Tests (2):
11:19:16     Swift(macosx-x86_64) :: Python/python_lint.swift
11:19:16     Swift(macosx-x86_64) :: Python/build_swift.swift

Lint:

./utils/swift_build_support/swift_build_support/products/__init__.py:32:1: I100 Import statements are in the wrong order. from .swiftformat should be before from .swiftinspect
./utils/swift_build_support/swift_build_support/products/swiftformat.py:14:1: F401 'platform' imported but unused

build_swift:

--
...ssssssssssssssss............................................................................................................................................................................................................F...............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................ss..........................................................................................................................................................................................................ssssss..sssssss..ss..ssss...s.ss......sssssssssssssssssssssss.sss...s....s.s.sssssss...ssssssss
======================================================================
FAIL: test_expected_options_exhaustive (tests.build_swift.test_driver_arguments.buildbot_incremental_linux,llvm-only)
Test that we are exhaustively testing all options accepted by the
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/utils/build_swift/tests/build_swift/test_driver_arguments.py", line 419, in test_expected_options_exhaustive
    .format(diff))
AssertionError: non-exhaustive expected options, missing: set([u'--swiftformat '])

----------------------------------------------------------------------
Ran 1002 tests in 32.681s

ahoppen

Overall looks good to me, I wrote a couple suggestions and explanations inline.

utils/build-presets.ini

ahoppen · 2020-06-26T18:35:57Z

utils/swift_build_support/swift_build_support/products/swiftformat.py

+            script_path,
+            action,
+            '--toolchain', self.install_toolchain_path(),
+            '--config', configuration,


I think you named this parameter --configuration, not --config in build-script-helper.py of swift-format. I am slightly inclined to change the parameter definition in the swift-format so it’s the same as for the stress tester, but don’t have strong opinions on it.

Good catch, but it should be the other way: we should update the stress tester to use --configuration to match the swift-build option name.

Thanks for catching this. I updated the name here.

ahoppen · 2020-06-26T18:43:12Z

utils/swift_build_support/swift_build_support/products/swiftformat.py

+            # 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'


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

You build swift-format using this build script, Package.resolved gets set to the local dependencies.

You open the project in Xcode. SWIFTCI_USE_LOCAL_DEPS is not set and thus Package.resolved gets reset to use remote dependencies.

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.

- Fix extra space in build-script arg to build swift-format. - Fix python lint errors in `swiftformat.py`. - Remove swiftsyntax from swiftformat build preset.

benlangmuir · 2020-06-26T21:50:15Z

@swift-ci please test

benlangmuir · 2020-06-26T21:50:30Z

@swift-ci please test

dylansturg · 2020-07-01T21:37:24Z

@benlangmuir Can you merge this? Or can anyone else? Thanks!

gottesmm reviewed Jun 25, 2020

View reviewed changes

akyrtzi requested a review from benlangmuir June 25, 2020 17:49

benlangmuir reviewed Jun 25, 2020

View reviewed changes

Minor review tweaks.

e56ce8a

- Remove unused `package_name` method. - Disable swift-syntax tests in swift-format builder preset.

swiftlang deleted a comment from swift-ci Jun 26, 2020

ahoppen reviewed Jun 26, 2020

View reviewed changes

Changes for review:

3b168d8

- Fix extra space in build-script arg to build swift-format. - Fix python lint errors in `swiftformat.py`. - Remove swiftsyntax from swiftformat build preset.

benlangmuir approved these changes Jun 29, 2020

View reviewed changes

benlangmuir merged commit c03ea7a into swiftlang:master Jul 1, 2020

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

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

Uh oh!

Conversation

dylansturg commented Jun 25, 2020

New Flags

Installing

Background

Uh oh!

allevato commented Jun 25, 2020

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benlangmuir commented Jun 26, 2020

Uh oh!

benlangmuir commented Jun 26, 2020

Uh oh!

swift-ci commented Jun 26, 2020

Uh oh!

benlangmuir commented Jun 26, 2020

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benlangmuir commented Jun 26, 2020

Uh oh!

benlangmuir commented Jun 26, 2020

Uh oh!

dylansturg commented Jul 1, 2020

Uh oh!

Uh oh!