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

Minor AppVeyor configuration cleanup #30

Merged
merged 1 commit into from
Apr 9, 2017

Conversation

Kamekameha
Copy link
Contributor

Apparently AppVeyor was ignoring the build_script section and building the visual-studio/12/dlfcn-win32.sln solution instead as its fallback of the build section not being defined. Setting the latter to off here would probably have been good enough though.

@traversaro
Copy link
Collaborator

Thanks a lot @Kamekameha . For some reason that I do not fully understand AppVeyor does not build PR, I will push your fix to a local branch to get AppVeyor running.

@Kamekameha
Copy link
Contributor Author

You could check if the appropriate event, the PR one specifically, is enabled for AppYeyor's webhook settings.

@traversaro
Copy link
Collaborator

I looked in the AppVeyor settings, but I could not find the right place, it could be related to the fact that I am not the owner of the organization, I am not sure.
Anyhow, I like how you cleaned up the AppVeyor script, but are you sure that the build_script is currently ignored? In the AppVeyor output (see for example https://ci.appveyor.com/project/dlfcn-win32/dlfcn-win32/build/1.0.1/job/adorate5a6to789d#L251 ) it is possible to see that the cmake configuration files are installed, and this can only happen if the CMake build is working correctly.

@Kamekameha
Copy link
Contributor Author

Yeah, I saw that. For some even weirder reason after I made the PR AppVeyor updated the configuration or something and rebuilt the latest commit. Did you by any chance restarted the latest build sometime after this PR? But yes, I'm 100% sure it was previously just building the VC12 solution this project has.

@traversaro
Copy link
Collaborator

I deleted and recreated the project to check if the PR webhook started working, but it did not, so unfortunately we lost all old builds. However I still have all the builds done when I was testing AppVeyor support in https://ci.appveyor.com/project/traversaro/dlfcn-win32/history , and it seems to be that the CMake project was correctly compiled for every build. Furthermore, the test_script also relies on the CMake build to be completed correctly, and it never failed in all the builds.

@Kamekameha
Copy link
Contributor Author

Hm, I see. Regarding losing the builds history, even before this PR there was only one build in the history, and it's still there.

I made the PR initially because I thought the .appveyor.yml file was misconfigured (and it kinda was for the os/image section) but it just seems AppVeyor failed to grab the settings correctly when that file was added; I never saw your repo's builds log, and now it does seem that AppVeyor is working as expected in this repo. At this point I guess we could close this PR, or if really desired, rename and rebase the title and the commit log message to something else.

@traversaro
Copy link
Collaborator

I think this PR is a reasonable cleanup of the AppVeyor configuration files structure, so please go ahead and change the commit name to something more informative such as "AppVeyor configuration cleanup" and I will be happy to merge the PR, thanks!

@Kamekameha Kamekameha changed the title Make sure AppVeyor actually builds the CMake project Minor AppVeyor configuration cleanup Mar 19, 2017
@Kamekameha
Copy link
Contributor Author

Kamekameha commented Mar 19, 2017

There we go.

By the way, is AppVeyor properly configured or whatever for this repo? I mean, your latest commit didn't trigger the build and this PR hasn't been built either. Also for reference, https://ci.appveyor.com/project/Kamekameha/dlfcn-win32/history; it seems to work on my fork and it does on yours as well, that's why I'm wondering.

@traversaro
Copy link
Collaborator

Hi @Kamekameha ,
I think the AppVeyor PR problems are due to #33 .
Anyway, thanks for the contribution and sorry for the delay!

@traversaro traversaro merged commit 18195b1 into dlfcn-win32:master Apr 9, 2017
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.

None yet

2 participants