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

🔨 Update tools for development on Apple Silicon #334

Merged
merged 21 commits into from
Mar 29, 2021
Merged

🔨 Update tools for development on Apple Silicon #334

merged 21 commits into from
Mar 29, 2021

Conversation

chris-araman
Copy link
Contributor

@chris-araman chris-araman commented Mar 22, 2021

Ruby 2.4.3 is no longer in support and fails to compile with current tools. The danger and xcpretty gems seem to run successfully on Ruby 3.0.0.

The swiftenv formula is not available in the homebrew/core tap, so we now specify its tap as kylef/formulae, per instructions at https://swiftenv.fuller.li/.

An arm64 shellcheck bottle is not available because ghc is not yet available for arm64 via Homebrew (Homebrew/homebrew-core#65997). I've added shfmt in the meantime, as it is complementary. I'll be happy to add shellcheck back once it's available for arm64.

Some of the scripts were indenting with 4 spaces, and others with 2. I decided to standardize on 2 for brevity, but am happy to change it if others disagree.

I thought it might be a good idea to install the Swift linters via Homebrew and remove our dependence on Mint. The linters weren't all available from Homebrew when first added to this project, but they are now. Mint is currently problematic on M1 Macs because it fails to link globally to /usr/local/bin when that path does not yet exist. Homebrew for M1 Macs defaults to /opt/homebrew/bin. One thing we lose here is the ability to pin to particular versions of the linters, as Homebrew only supports installing the "current" version. I think that's a fair trade-off.

The Homebrew bundle lockfile isn't a typical lockfile in that it doesn't support version pinning, however it does add some value in that it provides evidence of a last-known-good state. However, it doesn't seem to handle removed or renamed brews/taps/mas entries. To work around that oversight, I've started wiping it during mas build bootstrapping. This allows us to maintain a clearer picture of the last-known-good Homebrew bundle state. However, I'd be happy to remove this "lockfile" and start passing --no-lock instead, if others feel it isn't meeting our needs.

The sort scripts were used when CocoaSeeds were still in use, but references to both have previously been removed.

I've enabled the swiftformat and swift-format linters in addition to the existing swiftlint linter. Getting them to agree on certain rules took a bit of finesse.

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Bad exclusion: script/sort.pl
Bad exclusion: script/sort.pl

@chris-araman chris-araman requested a review from phatblat March 22, 2021 00:17
Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Bad exclusion: script/sort.pl
Bad exclusion: script/sort.pl

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Loading configuration from '.swiftlint.yml'
Loading configuration from '.swiftlint.yml'
Invalid configuration for 'opening_brace'. Falling back to default.
Linting Swift files at paths MasKitTests/Extensions/Bundle+JSON.swift
Linting 'Bundle+JSON.swift' (1/1)

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Loading configuration from '.swiftlint.yml'
Loading configuration from '.swiftlint.yml'
Invalid configuration for 'opening_brace'. Falling back to default.
Linting Swift files at paths MasKitTests/Extensions/Bundle+JSON.swift
Linting 'Bundle+JSON.swift' (1/1)

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Loading configuration from '.swiftlint.yml'
Loading configuration from '.swiftlint.yml'
Invalid configuration for 'opening_brace'. Falling back to default.
Linting Swift files at paths MasKitTests/Extensions/Bundle+JSON.swift
Linting 'Bundle+JSON.swift' (1/1)

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Loading configuration from '.swiftlint.yml'
Loading configuration from '.swiftlint.yml'
Invalid configuration for 'opening_brace'. Falling back to default.
Linting Swift files at paths MasKitTests/Extensions/Bundle+JSON.swift
Linting 'Bundle+JSON.swift' (1/1)

@chris-araman
Copy link
Contributor Author

chris-araman commented Mar 22, 2021

Some files could not be reviewed due to errors:
Loading configuration from '.swiftlint.yml'
Invalid configuration for 'opening_brace'. Falling back to default.

Hound supports only SwiftLint 0.27.0 from July 2018. The current release is 0.43.1 from last week.
http://help.houndci.com/en/articles/2461415-supported-linters
/~https://github.com/realm/SwiftLint/releases/tag/0.27.0
/~https://github.com/realm/SwiftLint/releases/tag/0.43.1

I've disabled SwiftLint in .hound.yml as Lint is already covered by Jenkinsfile. If you'd rather I try to make the configuration work for both versions, please let me know.

@chris-araman
Copy link
Contributor Author

Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

These appear to be a disagreement between Hound's older version of SwiftLint and the current version.

@chris-araman
Copy link
Contributor Author

@phatblat, is Jenkins healthy? It does look like builds have been successful there lately. I don't want to risk regressions to the Jenkins build, but I don't know what the healthy baseline should be.
https://jenkins.log-g.co/job/mas-cli/job/mas/view/change-requests/builds

@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@mas-cli mas-cli deleted a comment from houndci-bot Mar 27, 2021
@phatblat
Copy link
Member

Maybe I should change script/lint to autoformat (for developer convenience) and also to lint and fail (for CI)?

I would like to have a check fail when the linter returns errors. Could you split out the formatters into their own script? Then lint can run in a GitHub Action and format (or whatever it's called) can just be run manually by devs.

@chris-araman
Copy link
Contributor Author

Sounds good. I'll split out lint and format in some additional commits.

I've rebased against master.

@chris-araman
Copy link
Contributor Author

Hrm... I'm not sure what the build-test check is waiting on here. It's already passed:
/~https://github.com/chris-araman/mas/actions/runs/696277174

@phatblat
Copy link
Member

Oh, I think it's confused since you submitted this one from a fork. I'll override to get this big PR in before the other ones.

@chris-araman now that you're a contributor, you can create branches right on the mas repo which might work better.

@phatblat phatblat merged commit cf9ec27 into mas-cli:master Mar 29, 2021
@chris-araman chris-araman deleted the build branch March 29, 2021 01:43
@phatblat phatblat added this to the Unreleased milestone Mar 29, 2021
@phatblat phatblat modified the milestones: Unreleased, 1.8.2 Oct 19, 2021
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