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

build + ci GitHub action #799

Closed
wants to merge 12 commits into from
Closed

Conversation

aminya
Copy link

@aminya aminya commented May 21, 2020

Here We go! 🚀 All the binaries back to version 7 for Linux:
https://github.com/aminya/include-what-you-use/actions/runs/120620734

Version 6 and 5 building seem incomplete and because of that, the tests fail because the build folder is missing. Fixing the test for old versions may not be worth the time. I let the build folder get uploaded for all versions so if we want to investigate the issue.

We can make binaries for Windows and MacOS in a separate PR.

Fixes #798

@aminya
Copy link
Author

aminya commented May 21, 2020

@kimgr There seems to be an issue in building the project.

https://github.com/aminya/include-what-you-use/runs/697655108?check_suite_focus=true#step:10:26

CMake Error at /usr/lib/llvm-9/lib/cmake/clang/ClangTargets.cmake:588 (message):
  The imported target "clangBasic" references the file

     "/usr/lib/llvm-9/lib/libclangBasic.a"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "/usr/lib/llvm-9/lib/cmake/clang/ClangTargets.cmake"

  but not all the files it references.

Call Stack (most recent call first):
  /usr/lib/llvm-9/lib/cmake/clang/ClangConfig.cmake:19 (include)
  CMakeLists.txt:16 (find_package)

@aminya aminya force-pushed the binaries branch 12 times, most recently from ec79c41 to 185c09c Compare May 21, 2020 20:55
@aminya aminya force-pushed the binaries branch 2 times, most recently from 70e6108 to 2ac1989 Compare May 21, 2020 21:29
@kimgr
Copy link
Contributor

kimgr commented May 22, 2020

@aminya Thanks! I'm pretty happy with the travis build, but the integration with Github seems like it would be even better, especially if we can support more platforms and generate preliminary binaries.

Judging from the error message, I would guess that you're missing one or more clang packages -- the packaging for Ubuntu is a little wonky, so the Clang cmake configs come with one package, and the binaries with another, if I remember correctly. Check the travis config for the definitive package deps: https://github.com/include-what-you-use/include-what-you-use/blob/master/.travis.yml

@aminya aminya force-pushed the binaries branch 10 times, most recently from 5489ebd to 1b96afb Compare May 31, 2020 10:05
@aminya
Copy link
Author

aminya commented May 31, 2020

Here We go! 🚀 All the binaries back to version 7 for Linux:
https://github.com/aminya/include-what-you-use/actions/runs/120620734

Version 6 and 5 building seem incomplete and because of that, the tests fail because the build folder is missing. Fixing the test for old versions may not be worth the time. I let the build folder get uploaded for all versions so if we want to investigate the issue.

We can make binaries for Windows and MacOS in a separate PR.

@aminya aminya marked this pull request as ready for review May 31, 2020 14:06
@aminya
Copy link
Author

aminya commented May 31, 2020

You create releases using these binaries. We need to add them to Readme and website too.

@aminya aminya requested a review from kimgr May 31, 2020 14:13
@kimgr
Copy link
Contributor

kimgr commented May 31, 2020

Nice!

I'd like to suggest a slightly different design, though --

  • Only build master version with whatever LLVM version that is targeting (currently 11)
  • Backport an adjusted build definition to each release branch

The benefit is that we don't need to clutter master with all historical build designs/llvm buildsystem quirks. Rather than parameterizing the build to work with all past and future versions of IWYU, focus on the present and keep that simple, so that it's easier to change in the future.

So I say let's get a Linux build for master working, and get that in place to replace Travis for CI.

Then try to get macOS working. I think the test suite is still broken on macOS, but I've never had a Mac, so I've never been able to debug/troubleshoot it myself. We should probably put in a conditional pass/fail depending on test output here, so we can only fail the build if the test suite gets worse.

Then try to get Windows working. I know for a fact the test suite is broken there, so the same strategy should hold.

Once everything is in place, we can backport to clang_10, adjust the LLVM+Clang details and the pass/fail criteria for the test suite until it works.

And then on to older versions. I'm not sure we need to go further back than 8.

Does that sound reasonable?

@aminya
Copy link
Author

aminya commented Jun 1, 2020

The purpose of this pull request was not to replace the CI script that this repository is using, but to write a single build script that builds binaries across multiple versions. I don't call this "cluttering the master" but "backward compatibility".

This workflow runs only for named releases and branches. This build script is only meant to run whenever you tag a branch. You should add the LLVM version here once you tag it:

        llvm_version:
	      - 11.0.0
          - 10.0.0
 		  - ...

and then push a commit with having [build] in its commit message.

This is the same approach as the main apt.llvm bash script. Their script accepts input for installing a specific version.

However, I can make a workflow for CI too, which uses different logic for installing, downloading, determining the version, etc.

@aminya aminya force-pushed the binaries branch 2 times, most recently from 05df983 to 2f24732 Compare June 1, 2020 01:49
@aminya
Copy link
Author

aminya commented Jun 1, 2020

I added a CI workflow replacing Travis.
Here you can see all the workflows:
https://github.com/aminya/include-what-you-use/actions

@aminya aminya changed the title build GitHub action build + ci GitHub action Jun 1, 2020
@aminya
Copy link
Author

aminya commented Jul 14, 2020

@kimgr Any thought on this? I added the CI workflow as you requested.

@kimgr
Copy link
Contributor

kimgr commented Aug 2, 2020

Hey @aminya, thanks for working on this!

I'm still on the fence as to whether I actually want to produce official builds of IWYU -- I would much prefer to make it easier for packagers to maintain proper official per-platform packages.

The proposed workflow looks a little quirky to me -- would someone need to push a commit with [build] in the message to trigger a build? That seems strange; where does that commit go afterwards?

I think I would prefer to leave things as they are, and I might scavenge your PR to get a CI for all relevant platforms up and running. That would benefit the IWYU project, and we can let packagers work on producing binaries that are compatible with the various ABI variations for their respective platforms. I don't have bandwidth to support prebuilt binaries as things currently stand.

So it's a 👎 from me, but a 🙌 of thank you for working through it!

@kimgr kimgr closed this Oct 27, 2020
@aminya
Copy link
Author

aminya commented Oct 27, 2020

@kimgr I had not seen this. Even if you don't like build.yml, why don't you use the CI workflow? It perfectly replaces Travis.

@kimgr
Copy link
Contributor

kimgr commented Oct 27, 2020

Thanks! I was hoping to do so, but I have some other things before on my todo list. I'll revisit the ci when I get a chance

@aminya
Copy link
Author

aminya commented Nov 5, 2021

One year later after my rejected contribution and still no binaries. Are there any plans?

@kimgr
Copy link
Contributor

kimgr commented Nov 10, 2021

No, providing binaries is not a goal of the IWYU project itself; we can barely keep up with supporting source releases.

What platform are you on? Is there any packaging system available? If so, is there anything we can do to simplify that packaging?

@jonesmz
Copy link

jonesmz commented Nov 10, 2021

What additional burden does automatically building binaries with github CI cause?

@kimgr
Copy link
Contributor

kimgr commented Nov 11, 2021

@jonesmz My main concern is having to support "I downloaded this binary and it doesn't run on my system" kind of issues. There are two primary concerns that make it very hard (tm) for users to get this right on first try:

  • dependency on builtin headers (see issue 100)
  • dependency on libraries (we probably could force static link to llvm/clang, but its downstream dependencies are out of our control)

Having users build from source makes all of this more explicit. And packagers have infrastructure to express and enforce deps in a way that we don't with a single archived binary.

(Gah, typing on my phone. Editing to remove crazy duplication)

@aminya
Copy link
Author

aminya commented Nov 11, 2021

@jonesmz My main concern is having to support "I downloaded this binary and it doesn't run on my system" kind of issues.

Well, now no one even thinks about using this tool due to its overly high bar. This is similar to the Gentoo situation. The people daily driving Gentoo are rare.

dependency on builtin headers (see issue 100)
dependency on libraries (we probably could force static link to llvm/clang, but its downstream dependencies are out of our control)
Having users build from source makes all of this more explicit. And packagers have infrastructure to express and enforce deps in a way that we don't with a single archived binary.

This is just handing over the issues. This tool has dependencies, and you want all the users to inherit this. While it could be just solved by providing prebuilt binaries.

@jonesmz
Copy link

jonesmz commented Nov 11, 2021

@jonesmz My main concern is having to support "I downloaded this binary and it doesn't run on my system" kind of issues.

Those are good concerns, thank you.

My personal opinion is that the burden of getting source builds working for some arbitrary end user is substantially alleviated if there was a github ci that built iwyu and ran it through a small number of regression tests.

You don't even have to publish the resulting binary. Just having the build automated is enough to really cut down on the work.

@jonesmz
Copy link

jonesmz commented Nov 11, 2021

Well, now no one even thinks about using this tool due to its overly high bar. This is similar to the Gentoo situation. The people daily driving Gentoo are rare.

Amusingly I am a Gentoo user, trying to get iwyu to work on windows. It took me all of 5 minutes to get it working in Gentoo the first time. Still haven't gotten it working in Windows yet, though largely because I'm juggling too many projects.

@kimgr
Copy link
Contributor

kimgr commented Nov 30, 2021

My personal opinion is that the burden of getting source builds working for some arbitrary end
user is substantially alleviated if there was a github ci that built iwyu and ran it through a small
number of regression tests.

There is. Or you mean for Windows specifically? Yeah, it would be nice to get a Windows CI in place, though I'm not sure what the story is for packages on Windows these days.

When I used Windows many moons ago, I found it impressively straightforward to build Clang/LLVM (and eventually IWYU) there, it just took a long time. Compared to other open source projects on Windows, it really was a breeze. Maybe other projects are even better these days with better tooling?

I'd rather not talk about the IWYU project providing binaries, because I don't think that's sustainable. I would love to discuss:

  • concrete pain points/difficulties in building on Windows
  • how to set up a Windows CI
  • how to kick-start and support packaging for Windows separately from the IWYU project

@aminya
Copy link
Author

aminya commented Nov 30, 2021

I decided to fork this project and provide the binaries. If you do not want to do it, I will take responsibility.

The build for Linux and MacOS was not that hard. However, I tried to build the windows version of include-what-you-use for a couple of weeks. But it is incredibly hard to do it. I will probably make another round of attacks, but not sure if this is possible. It is not clear, what project structure find_package(CLANG) expects. Should it be an llvm-project structure, an llvm release structure, or a clang sub-folder structure?
https://github.com/aminya/include-what-you-use-bin/tree/clang_12

Here is the list of jobs I tried
https://github.com/aminya/include-what-you-use-bin/actions

@kimgr
Copy link
Contributor

kimgr commented Nov 30, 2021

@aminya Thanks for taking it on!

find_package(CLANG) expects a clang install tree rooted in CMAKE_PREFIX_PATH. So on Debian-like systems after installing the packages from https://apt.llvm.org/, the directory tree starts in /usr/lib/llvm-14 (or whatever version suffix), and that tree contains headers, libraries and CMake modules for the LLVM/Clang distribution.

Incidentally, that installed tree has the same structure as the build tree for LLVM/Clang, so if you build it locally from source in ~/llvm/out, you can use the build root as your prefix path:

include-what-you-use/build$ cmake -DCMAKE_PREFIX_PATH=/usr/lib/llvm-14 ..

or

include-what-you-use/build$ cmake -DCMAKE_PREFIX_PATH=~/llvm/out ..

should both be viable (assuming all the right projects have been built in ~/llvm/out).

So it looks like your CI downloads LLVM source. But what you actually want is a packaged LLVM. I don't know if that exists for Windows. The alternative is to start from LLVM source, then build all of LLVM+Clang and then configure IWYU against the build tree, as described above.

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.

Binaries?
3 participants