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

[Linter] Grey areas #44

Closed
joshkalpin opened this issue Dec 2, 2013 · 20 comments
Closed

[Linter] Grey areas #44

joshkalpin opened this issue Dec 2, 2013 · 20 comments

Comments

@joshkalpin
Copy link
Member

I'm sure everyone can pipe in, but with the number of linter improvements I've made I'd like to see if we can make a bit more of a concrete list of areas that we should be checking for inside of it. This will also help keep things a bit more organized/documented on what we need to work on in this area.

cc/ @irrationalfab, @alloy, @Keithbsmiley, @orta

@fabiopelosin
Copy link
Member

Is this issue about listing the areas of improvement to the linter?

If so heard are my 2 cents:

  • The linter was rushed in a big refactor so some specs are disabled it would be great to fix those
  • The linter should check YAML podspecs for any unknown key (a commented implementation which was broken in the rush should be available at the end of the file)

@keith
Copy link
Member

keith commented Dec 2, 2013

  • It would be nice to check for the tag in question in a spec and possibly offer similar ones if the specific one didn't exist. Rather than git crashing when attempting to check it out.
  • It would be nice to see [ERROR] or something beside the specs in the summary list so you could CMD+F for what has broken the master build.

@joshkalpin
Copy link
Member Author

@irrationalfab yes, that's exactly what this issue is for.

@joshkalpin joshkalpin reopened this Dec 2, 2013
@alloy
Copy link
Member

alloy commented Dec 3, 2013

  • Ensure the libraries and vendored_libraries attribute contains e.g. ‘foo’ and not ‘libFoo.a’.
  • Ensure the vendored_frameworks attribute contains e.g. ‘Foo’ and not ‘Foo.framework’.
  • Disallow the use of the vendored_frameworks for iOS specs. These are not frameworks, but instead are static libraries disguised as frameworks and this makes it harder to use these specs outside of the context of Xcode. E.g. with clang from the command-line. Ideally this would also check the xcconfig attribute to ensure it’s not adding any FRAMEWORKS_SEARCH_PATHS.

@orta
Copy link
Member

orta commented Dec 3, 2013

Disallow the use of the vendored_frameworks for iOS specs. These are not frameworks, but instead are static libraries disguised as frameworks and this makes it harder to use these specs outside of the context of Xcode. E.g. with clang from the command-line. Ideally this would also check the xcconfig attribute to ensure it’s not adding any FRAMEWORKS_SEARCH_PATHS.

What a bad-ass.

@fabiopelosin
Copy link
Member

Disallow the use of the vendored_frameworks for iOS specs. These are not frameworks, but instead are static libraries disguised as frameworks and this makes it harder to use these specs outside of the context of Xcode. E.g. with clang from the command-line. Ideally this would also check the xcconfig attribute to ensure it’s not adding any FRAMEWORKS_SEARCH_PATHS.

What a bad-ass.

http://i.imgur.com/Ql8B0.gif

@alloy
Copy link
Member

alloy commented Dec 4, 2013

lol, you guys.

@keith
Copy link
Member

keith commented Dec 12, 2013

When a spec is update that is part of the white list, errors are still shown in the build. https://travis-ci.org/CocoaPods/Specs/builds/15322032

@fabiopelosin
Copy link
Member

@Keithbsmiley My idea is that the white list should go away sooner or later.

To all: before converting the specs repo to the new format we should get rid of the hooks which are not supported anymore.

@keith
Copy link
Member

keith commented Dec 13, 2013

I would love to clean up in that area. Not sure what you want to do though. Removing stuff I suppose

@fabiopelosin
Copy link
Member

Not sure about how to proceed with the git tags. Regarding the hooks we can port them to a pre-install hook if they are simple enough or remove them... thoughts?

@keith
Copy link
Member

keith commented Dec 14, 2013

Looks like you'd have to validate tags with some form of a git ls-remote --tags command. This get's all the tag names separated by spaces from the command line:

git ls-remote --tags --quiet | cut -f 2 | awk '{ split($0,a,"/"); print a[3]; }' | xargs

There are still 68 specs using pre_install (not showing versions they are):

CocoaLibSpotify CoconutKit CorePlot DTCoreText Facebook-iOS-SDK HockeySDK LevelDB-ObjC MapBox ReactiveCocoa SinglySDK Three20 ctemplate freexl geos icu4c lambert-objc libetpan libsasl2 spatialite

The 81 specs using post_install:

ARCHelper ARCMacro AppPaoPaoSDK CocoaLibSpotify CoconutKit DTCoreText Facebook-iOS-SDK GrannySmith HockeySDK LibComponentLogging-Core LibComponentLogging-Crashlytics LibComponentLogging-LogFile LibComponentLogging-NSLog LibComponentLogging-NSLogger LibComponentLogging-SystemLog LibComponentLogging-UserDefaults LibComponentLogging-pods LibComponentLogging-qlog MKStoreKit MagicalRecord MapBox QuickDialog SSToolkit SYCache TWReverseAuth TouchDB XingSDK unoffical-twitter-sdk

@keith
Copy link
Member

keith commented Dec 14, 2013

CocoaLibSpotify has a full fledged python script as a pre_install hook. https://github.com/CocoaPods/Specs/blob/master/CocoaLibSpotify/2.4.2/CocoaLibSpotify.podspec

@fabiopelosin
Copy link
Member

@Keithbsmiley Lol. But that means that we can just copy and paste it in the prepare_command.

@keith
Copy link
Member

keith commented Dec 14, 2013

Yea with stuff like this, https://github.com/CocoaPods/Specs/blob/master/CoconutKit/2.0.2/CoconutKit.podspec#L26-L55 I don't think we can remove them

@fabiopelosin
Copy link
Member

@Keithbsmiley the import of the header can be done with a shell script similarly to what I have done in the ChatCore podspec. Regarding the resource bundles the spec is working around CocoaPods which at the time didn't support them. It is likely that more modern version of the spec have the proper setup for the bundles and thus that part can just be ported.

The problem is that this process is time consuming, especially if we want to lint/test the podspec. What about pinging the authors and asking them to fix the hooks, they could help us a lot.

@keith
Copy link
Member

keith commented Mar 14, 2014

public_header_files should not allow anything but headers CocoaPods/Specs#9238 (comment)

@joshkalpin
Copy link
Member Author

Still tracking these, just school slammed right now. Hoping I can get on it near the end of April.

@fabiopelosin fabiopelosin changed the title Linter Grey Areas [Linter] Grey areas Mar 31, 2014
@fabiopelosin
Copy link
Member

Two importants point to not forget:

@fabiopelosin
Copy link
Member

As this issue was becoming a kitchen sink hard to track I've spitted it up in smaller ones:

The linter should check YAML podspecs for any unknown key (a commented implementation which was broken in the rush should be available at the end of the file)

Moved to #88

public_header_files should not allow anything but headers CocoaPods/Specs#9238 (comment)

Moved to #89

Ensure the libraries and vendored_libraries attribute contains e.g. ‘foo’ and not ‘libFoo.a’.
Ensure the vendored_frameworks attribute contains e.g. ‘Foo’ and not ‘Foo.framework’.

Moved to #90

Disallow the use of the vendored_frameworks for iOS specs. These are not frameworks, but instead are static libraries disguised as frameworks and this makes it harder to use these specs outside of the context of Xcode. E.g. with clang from the command-line. Ideally this would also check the xcconfig attribute to ensure it’s not adding any FRAMEWORKS_SEARCH_PATHS.

Moved to #91

It would be nice to check for the tag in question in a spec and possibly offer similar ones if the specific one didn't exist. Rather than git crashing when attempting to check it out.

Moved to CocoaPods/CocoaPods#2006

It would be nice to see [ERROR] or something beside the specs in the summary list so you could CMD+F for what has broken the master build.

Moved to CocoaPods/CocoaPods#2007

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants