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

Case‐Sensitive File Systems #19

Merged
merged 7 commits into from Nov 8, 2017
Merged

Case‐Sensitive File Systems #19

merged 7 commits into from Nov 8, 2017

Conversation

SDGGiesbrecht
Copy link
Contributor

Fixes #14.

Actually, since there are no tests, it is hard to tell if I dealt with every possible casing issue, but I did fix everything that caught my eye, and I can now guarantee that Mint can successfully run...

$ mint run realm/SwiftLint

...which I imagine involves most operations Mint performs.

Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Nice one SDGGiesbrecht!

Package.swift Outdated
@@ -22,7 +22,7 @@ let package = Package(
// Targets are the basic building blocks of a package. A target can define a module or a test suite.
// Targets can depend on other targets in this package, and on products in packages which this package depends on.
.target(
name: "Mint",
name: "mint",
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of changing the target name to lowercase, I'd rather specify an explicit product with a lowercase name

.executable(name: "mint", targets: ["Mint"]),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yonaskolb, I will double check, but I am pretty sure that the name argument for products is only used in the manifests of other packages where they specify which of your products to actually use. I think it is the target name that defines the name of the executable file itself, and hence what you use from the shell.

If that is correct, then it would be reasonable to make the executable name uppercase if you want (for referencing in manifests), but the target name needs to lowercase unless you want the command to always be used capitalized, i.e. $ Mint install ..., which would be a little unusual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tested it. My assumption was wrong. Holdover from Swift 3 I guess.

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 will fix it.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you rename the mint folder back to Mint as well then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it only sort of works. The executable file gets the product name, but when the executable file differs from the corresponding target name (which I imagine is rare) some of swift package’s subcommands start to have trouble. So I would still highly recommend using mint for both the target and product (if you want to start exposing it as a product).

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting. What sort of subcommands have trouble?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

describe, dump-package, etc.

$ brew install mint
```

### Make

```sh
$ git clone https://github.com/yonaskolb/mint.git
$ cd mint
$ git clone https://github.com/yonaskolb/Mint.git
Copy link
Owner

Choose a reason for hiding this comment

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

I could also change the repo name to be lowercase. What do you think? I think I prefer uppercase, but could be swayed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no reason not to keep the repository and package names capitalized. That is the way most projects are and I think it looks nicer.

@yonaskolb
Copy link
Owner

To fix the issue with SwiftLint from #14, you would need to remove the lowercased() here as well https://github.com/yonaskolb/Mint/blob/master/Sources/Mint/main.swift#L66
Otherwise it will look for command swiftlint

@SDGGiesbrecht
Copy link
Contributor Author

Otherwise it will look for command swiftlint

...which is correct in SwiftLint’s case, and for the lion’s share of other tools as well.

@yonaskolb
Copy link
Owner

Yeah, removing that lowercase will fix other libraries. I assumed SwiftLint was capital case because the error you posted in #14 said:

Error: Couldn't find command swiftlint

@SDGGiesbrecht
Copy link
Contributor Author

That error was because the .build/Release/ folder did not exist to copy anything out of. (It is really .build/release.)

@SDGGiesbrecht
Copy link
Contributor Author

@yonaskolb,

Summary of the decisions you need to make before I can do anything else.

(Case‐insensitive file systems will never notice the difference no matter what you decide.)

  1. Which do you want users to do?
  • $ mint install ...
  • $ Mint install ...
  1. How do you want to guess the executable’s name (at least until Get Executable Name(s) etc. from Package Manifest #17)?
  • Fold to lowercase. (Fails for row 4.)
  • Identical to package name. (Fails for rows 2 and 3.)

For reference, here is the casing of the names of other Swift‐related tools, including all the tools in Mint’s read‐me:

Documentation Package Target Directory Product Command
(actual)
Command
(read‐me)
Swift
Xcode
CocoaPods
Git
Homebrew
Jazzy
Slather
Swift Version Manager
Travis Client
swift
xcodebuild
pod
git
brew
jazzy
slather
swiftenv
travis
Carthage
Mint (pr)
SourceKitten
SwiftLint
Workspace
carthage
mint
sourcekitten
swiftlint
workspace
Marathon
Mint (master)
XcodeGen
marathon†
mint
xcodegen†
danger-swift Runner danger-swift
SwagGen

†Wherever “Command (actual)” does not match “Command (read‐me)”, the tool appears broken on case‐sensitive file systems, since all examples in the read‐me and documentation just result in command not found. (@JohnSundell, that includes Marathon, in case you are interested.)

@yonaskolb
Copy link
Owner

I would like the executable to be lowercase. If it's possible to still have the target name be capitalised in SPM that's great, otherwise it's ok to sacrifice that and also make it also lowercase.

In terms of parsing the executable name, we should just leave the value untouched instead of lowercasing it. If we don't find the command with the casing given, we should try using lowercase.
When #17 is complete, the value will be unused or as a last resort fallback

@SDGGiesbrecht
Copy link
Contributor Author

The target name is capitalized again. It is an internal detail, so if problems arrive in the future, it will not be a breaking change to conform it to the executable name.


The executable name of a given tool is now presumed to be exactly the same as the package name.

It turns out trying fallback names (i.e. lowercase) would require a severe refactor, because right now the Package must be initialized with an executable name before the repository is ever retrieved. I therefore deem fallback guessing to be out of the scope of this pull request. For now, I instead fixed the examples in the read‐me to explicitly specify the executable name when it differs from the repository name (i.e. SwiftLint).

$ mint install yonaskolb/xcodegen # use newest tag
$ mint run yonaskolb/xcodegen@1.2.4 # run 1.2.4
$ mint run xcodegen # use newest tag and find xcodegen in installed tools
$ mint install yonaskolb/XcodeGen@1.2.4 "XcodeGen --spec spec.yml" # pass some arguments
Copy link
Owner

Choose a reason for hiding this comment

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

It reminds me to do the same lowercase product in XcodeGen 👍

- [yonaskolb/SwagGen](https://github.com/yonaskolb/SwagGen)
- $ mint run [realm/SwiftLint](https://github.com/realm/SwiftLint) swiftlint
- $ mint run [JohnSundell/Marathon](https://github.com/JohnSundell/Marathon)
- $ mint run [yonaskolb/XcodeGen](https://github.com/yonaskolb/XcodeGen)
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@yonaskolb yonaskolb merged commit 156b24f into yonaskolb:master Nov 8, 2017
@SDGGiesbrecht SDGGiesbrecht deleted the case‐sensitive branch November 10, 2017 01:06
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.

Inoperable on Case Sensitive File Systems
2 participants