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

Setup TVVLCKitTests #18

Closed
wants to merge 3 commits into from
Closed

Conversation

mkchoi212
Copy link
Contributor

Commits from #17 have been cherry-picked and will need to be rebased on it is merged

Copy link
Contributor

@fkuehne fkuehne left a comment

Choose a reason for hiding this comment

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

Nice work!

@fkuehne fkuehne requested a review from carolanitz June 7, 2018 13:32
@mkchoi212 mkchoi212 force-pushed the setup-tvos branch 2 times, most recently from 62d45ab to b4c6fc3 Compare June 14, 2018 08:43
Copy link
Member

@carolanitz carolanitz left a comment

Choose a reason for hiding this comment

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

I just have a couple of questions to understand parts of this PR. I don't want to blindly merge this :) and remarks for thought

@@ -19,14 +19,15 @@ jobs:
paths:
- .bundle
- restore_cache:
key: libvlc-v2-{{ checksum "compileAndBuildVLCKit.sh" }}
key: libvlc-v4-{{ checksum "compileAndBuildVLCKit.sh" }}
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why do you need to change that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CircleCI doesn't let you override old caches. With this PR, new directories regarding TVVLCKit must be cached. Also, I went from v2 to v4 because I messed up with v3 :p

CAA03D0C20C8DCDB001764DA /* libTVVLCKit.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 7D1329591BA304D900BE647E /* libTVVLCKit.a */; };
CAA03D1720C8DE4D001764DA /* libbz2.tbd in Frameworks */ = {isa = PBXBuildFile; fileRef = CAA03D1620C8DE4D001764DA /* libbz2.tbd */; };
CAA03D1920C8DE51001764DA /* libz.tbd in Frameworks */ = {isa = PBXBuildFile; fileRef = CAA03D1820C8DE51001764DA /* libz.tbd */; };
CAA03D1B20C8DE58001764DA /* libiconv.tbd in Frameworks */ = {isa = PBXBuildFile; fileRef = CAA03D1A20C8DE58001764DA /* libiconv.tbd */; };
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to add all of these frameworks?

Copy link
Contributor Author

@mkchoi212 mkchoi212 Jun 19, 2018

Choose a reason for hiding this comment

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

I had the hardest time trying to figure this out but @fkuehne mentioned here that

For the C++ errors - do you link against libc++ and all the other required libraries? See a dynamic framework target to get an idea.

The other required libraries he was talking about are these libraries. I'm currently writing tests in MobileVLCKitTests and I had to include all frameworks from DynamicMobileVLCKit as well.

0867D69AFE84028FC02AAC07 /* Frameworks */,
034768DFFF38A50411DB9C8B /* Products */,
CAA9EFFB20D24AEE00CDBB2C /* Recovered References */,
Copy link
Member

Choose a reason for hiding this comment

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

why do we need that folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't :D good catch

Rakefile Outdated
SCHEME_MAC = 'VLCKitTests'

VLC_FLAGS_IOS = '-dva x86_64'
VLC_FLAGS_TV = '-ftb'
Copy link
Member

Choose a reason for hiding this comment

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

would -stb be enough? we don't need it for a device here, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like using -stb fails to compile 🤔https://circleci.com/gh/videolan/vlckit/42

Copy link
Member

Choose a reason for hiding this comment

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

@Mikanbu fixed the underlying issue can we retry with stb here?

puts 'Found pre-existing build directory. Skipping build'
else
sh "./compileAndBuildVLCKit.sh #{VLC_FLAGS_TV}"
end
Copy link
Member

Choose a reason for hiding this comment

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

I am still not sure if we really need to skip the build here. I see this as a potential source for a wrong test outcome where we use an outdated build directory. We right now don't need to be fast or save time so our number 1 priority should just be an accurate test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true. But if we have the right build directory cached to start off with, I can't really see anything going wrong - since we aren't modifying anything in the build directory. Plus, if anything changes to the way we build VLC, the old cache would be ignored.

I think by caching the build directory, we have one less thing to worry about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What we could also do is keep using the cache for now and remove it once all tests are written. This way we don't have to wait for an hour to build & run tests while submitting minor patches/additions.

@mkchoi212 mkchoi212 force-pushed the setup-tvos branch 2 times, most recently from 265bed9 to d932a0b Compare June 19, 2018 06:33
@mkchoi212 mkchoi212 mentioned this pull request Jun 19, 2018
@mkchoi212 mkchoi212 force-pushed the setup-tvos branch 5 times, most recently from 4a571a0 to d6d41b8 Compare June 25, 2018 10:19
@carolanitz
Copy link
Member

carolanitz commented Jun 25, 2018

I'd say this is blocked by https://code.videolan.org/videolan/VLCKit/issues/178 or more this is exactly the reason why we need this test :D to catch this!

@carolanitz
Copy link
Member

this was merged with fc5e423

@carolanitz carolanitz closed this Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants