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 a separate print-api executable for each supported GHC versions #16

Merged
merged 9 commits into from
Sep 1, 2024

Conversation

mmhat
Copy link
Contributor

@mmhat mmhat commented Aug 10, 2024

Then, the print-api executable is a simple wrapper that detects the used GHC version and calls the appropriate print-api-VERSION exectuable which does the actual work.
The following works (inside the print-api source tree):

# Outputs 9.10.1 in my case
ghc --numeric-version

# Builds and installs print-api and print-api-9.8.2
cabal build print-api:exe:print-api-9.8.2 --with-compiler ghc-9.8.2

# Builds and installs print-api and print-api-9.10.1
cabal build all --with-compiler ghc-9.10.1

# We need to build the project once for each GHC; Otherwise print-api won't find the appropriate packagedb
cabal build all --with-compiler 9.8.2
cabal build all --with-compiler 9.10.1

All of these commands give the correct output using the GHC executable that is "in scope".
print-api -p print-api
ghcup run --ghc 9.8.2 -- print-api -p print-api
ghcup run --ghc 9.10.1 -- print-api -p print-api

I also checked in a install-for-ghcup-compilers.sh Bash script that installs the executable for all GHCs currently installed by ghcup.
I think this unblocks #13 to some degree, doesn't it?

Also: This tool is awesome! Thanks for writing it!

@Kleidukos
Copy link
Owner

@mmhat Hi! Thank you so much for your involvement in print-api. :)

Could you please update the release action to produce binaries for the wrapper as well? One per OS/Arch will be enough. :)

@mmhat
Copy link
Contributor Author

mmhat commented Aug 11, 2024

@Kleidukos Ok, there is only one issue left with the packaging: The binaries in print-api-0.1.0.1-Linux-x86_64.tar.gz release archive should be dynamically linked, but they are not. I couldn't figure out why, and given that I spent much more time today on this issue than I intended, this has to wait...

EDIT: Oh forgetful me, we run upx on the executables, and after that they appear to be statically linked (See https://stackoverflow.com/questions/20740625/does-upx-magically-transform-binaries-from-dynamically-linked-into-statically-li)...

@Kleidukos
Copy link
Owner

@mmhat Yes sorry, I was confused too at first but never wrote anything down about it in the repository.

@mmhat
Copy link
Contributor Author

mmhat commented Aug 11, 2024

@Kleidukos Ok, I think I am done here. I still need to update the documentation, but apart from that I am done here. Can you review the PR and tell me if there is something left to do for me?

Also, here are some quick remarks (not sure which of those should be an own issue or part of this PR):

  1. I think some parts of the CI can be consolidated: Right now we have two workflow containing two jobs. Those four jobs are awfully similar, and we might just merge them (or some of them) into one (or two) jobs/workflows in order to reduce duplication.
    Would such a contribution be accepted?
  2. There is some room for UX improvements: For example, we may want to add a --with-compiler flag to the wrapper executable to force the usage of a certain GHC compiler.
  3. An annoying issue of the current cabal exec implementation is that it requires that project was build at least once. See here for the upstream bug report. Not sure if there is something actionable we can do here; The only thing I can think of is a general suggestion to the user like Did you run cabal build --with-compiler VERSION ?.
  4. Since the wrapper/worker split of the executables is somewhat fragile we might want a testsuite checking that the print-api-VERSION gets picked.
  5. The wrapper/worker split of the executables yields some new points in the design space what print-api could do if a print-api-VERSION executable could not be found. We could extend the scope of it to be a downloader (i.e. download the appropriate bindist if a print-api-VERSION is not found) or an installer (i.e. download the source code, build and install the demanded print-api-VERSION if it is not found) or print instruction how to obtain it.
  6. Also, I think it was nice if the library provides the boiler plate to implement a Cabal testsuite checking API differences.

@mmhat Yes sorry, I was confused too at first but never wrote anything down about it in the repository.

I left a comment right above the release job in the hope that this fact is not forgotten.

@mmhat
Copy link
Contributor Author

mmhat commented Aug 11, 2024

Ok, I also updated the documentation 👍

Repository owner deleted a comment from tchoutri Aug 12, 2024
@Kleidukos
Copy link
Owner

  1. I think some parts of the CI can be consolidated: Right now we have two workflow containing two jobs. Those four jobs are awfully similar, and we might just merge them (or some of them) into one (or two) jobs/workflows in order to reduce duplication.
    Would such a contribution be accepted?

You're talking about the Tests and Release pipelines? If you find a way to do that, yes certainly, although perhaps in the future we'll want the test pipeline to go faster and the release pipeline to do more things, so if they can be kept modular that would be grand. :)

  1. There is some room for UX improvements: For example, we may want to add a --with-compiler flag to the wrapper executable to force the usage of a certain GHC compiler.

There is room indeed.

  1. The only thing I can think of is a general suggestion to the user like Did you run cabal build --with-compiler VERSION ?.

That's a good thing, yes.

  1. Since the wrapper/worker split of the executables is somewhat fragile we might want a testsuite checking that the print-api-VERSION gets picked.

Yes

  1. We could extend the scope of it to be a downloader (i.e. download the appropriate bindist if a print-api-VERSION is not found) or an installer (i.e. download the source code, build and install the demanded print-api-VERSION if it is not found) or print instruction how to obtain it.

No I believe that is a job that is best left to ghcup. Otherwise a proper message that asks the user to get the correct binary will suffice.

  1. Also, I think it was nice if the library provides the boiler plate to implement a Cabal testsuite checking API differences.

Do you have something clearer in mind? I could be interested. :)

@Kleidukos
Copy link
Owner

@mmhat You removed the nightly release mechanism from the CI, which is not quite what I expected from this PR

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@mmhat
Copy link
Contributor Author

mmhat commented Aug 13, 2024

@mmhat You removed the nightly release mechanism from the CI, which is not quite what I expected from this PR

... which is not quiet what I intended either 🙃

Just to clarify that we are talking about the same thing: By "nightly release" you mean the print-api-head-* artifacts uploaded during the ci.yml workflow?

@mmhat
Copy link
Contributor Author

mmhat commented Aug 14, 2024

@Kleidukos You may have noticed that a lot of lines changed with no apparent reason. I did that because I wanted to know what the actual differences between the ci.yml and the release.yml pipelines were. I went on and changed one or the other until they had a nice nvim -d ci.yml release.yml output showing those differences.
In the process I might have changed some things a bit too much though.

Regarding the workflow artifacts a.k.a nightly release:
Those are still uploaded, see https://github.com/Kleidukos/print-api/actions/runs/10340114025#artifacts.
However, the binaries contained in the ZIP file have a different naming pattern, for example print-api-0.1.0.1-... instead of print-api-head-.... I can change that if that is a problem.

Regarding the removal of the release job in the ci.yml:
As far as I can tell it only runs if a tag gets pushed, doesn't it? But in that case the release.yml workflow should be triggered as well. (Almost: I just realized that there is a tiny difference in the run conditions: The release job in the release.yml is just runs on tags that start with "v" while the one ci.yml would run for any tag.)
Also, my understanding is that the CI currently creates an ordinary release (not a pre-release) in both release.yml and ci.yml runs if a tag starting with "v" gets pushed.
Am I mistaken here? Is that the indented behaviour?

I hope that sheds some light on the changes in that PR...

@mmhat
Copy link
Contributor Author

mmhat commented Aug 14, 2024

You're talking about the Tests and Release pipelines? If you find a way to do that, yes certainly, although perhaps in the future we'll want the test pipeline to go faster and the release pipeline to do more things, so if they can be kept modular that would be grand. :)

I had nothing less in mind 🙂
I think the (biggest) differences between the two workflows (currently in main) are:

  1. Both run on pushes to main; release.yml if "v" tags are pushed and ci.yml runs for pull requests.
  2. ci.yml has only a GHC 9.10.1 Alpine job; release.yml runs the whole matrix for Alpine.
  3. Both have release jobs, but I believe the one in ci.yml never runs due to mutual excluding conditions. (I think I got this wrong in my previous comment.)

To me that looks like as if it could one workflow with just some run conditions for the release job and a conditional build matrix.

No I believe that is a job that is best left to ghcup. Otherwise a proper message that asks the user to get the correct binary will suffice.

I totally; Just wanted to point out the options here.

  1. Also, I think it was nice if the library provides the boiler plate to implement a Cabal testsuite checking API differences.

Do you have something clearer in mind? I could be interested. :)

Nothing specific yet; Essentially I want a defaultMain that dumps the API of the current package and compares it to a golden value.

@Kleidukos
Copy link
Owner

Just to clarify that we are talking about the same thing: By "nightly release" you mean the print-api-head-* artifacts uploaded during the ci.yml workflow?

Yes I am talking about the -head releases :)

@Kleidukos
Copy link
Owner

Both have release jobs, but I believe the one in ci.yml never runs due to mutual excluding conditions.

Ah, it appears I messed up the -head pre-reload workflow then. This will have to be fixed after this PR lands.

@Kleidukos
Copy link
Owner

Nothing specific yet; Essentially I want a defaultMain that dumps the API of the current package and compares it to a golden value.

Hmm, can't we do something that integrates with https://flora.pm/packages/@hackage/tasty-golden already?

install-for-ghcup-compilers.sh Outdated Show resolved Hide resolved
@mmhat
Copy link
Contributor Author

mmhat commented Aug 16, 2024

Both have release jobs, but I believe the one in ci.yml never runs due to mutual excluding conditions.

Ah, it appears I messed up the -head pre-reload workflow then. This will have to be fixed after this PR lands.

Can you open an issue with a short description how the workflows should work? Maybe I'll find the time to fix that too.

Also, I noted that the workflows in the get-tested seem to be identical (I just had a quick look, so I might be wrong). Should those be changed to?

@mmhat
Copy link
Contributor Author

mmhat commented Aug 16, 2024

Nothing specific yet; Essentially I want a defaultMain that dumps the API of the current package and compares it to a golden value.

Hmm, can't we do something that integrates with https://flora.pm/packages/@hackage/tasty-golden already?

Maybe; I don't know (yet). It was just an idea while I was working on this PR.

@mmhat mmhat force-pushed the wrapper branch 2 times, most recently from 7ca797c to e9e7cf7 Compare August 21, 2024 00:46
@mmhat
Copy link
Contributor Author

mmhat commented Aug 21, 2024

@Kleidukos I improved the install-for-ghcup-compilers.sh script as requested. Is there anything else I can do to push this PR over the line?

@Kleidukos
Copy link
Owner

@mmhat yes, two things:

  1. There are unfortunately conflicts now, sorry.
  2. If you could restore the pre-release mechanism that you removed, it would help with reviewing because the diff would be smaller. Let's discuss this in another ticket, as you suggested. :)

mmhat added 7 commits August 22, 2024 02:47
The `print-api` executable is a simple wrapper that detects the used GHC
version and calls the appropriate `print-api-VERSION` exectuable which
does the actual work.
@mmhat
Copy link
Contributor Author

mmhat commented Aug 22, 2024

@mmhat yes, two things:

  1. There are unfortunately conflicts now, sorry.

No problem.

  1. If you could restore the pre-release mechanism that you removed, it would help with reviewing because the diff would be smaller. Let's discuss this in another ticket, as you suggested. :)

I added the pre-release job as it is currently in the main branch. Does that meet your expectations or should I roll back more changes made in the CI?

Copy link
Owner

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

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

Thank you for your patience @mmhat. This is a valuable contribution. These are my last remarks. If something was noticed in one workflow file it's also valid for the other.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
install-for-ghcup-compilers.sh Outdated Show resolved Hide resolved
@mmhat
Copy link
Contributor Author

mmhat commented Aug 31, 2024

@Kleidukos The diff of the workflow files should be much smaller now.
Overall only the following changed in those:

  1. I inlined the two scripts. This is the only change that is not necessary.
  2. The post-process step for ubuntu-latest strips all executable.
  3. The packaging step packs all executables.
  4. The release job was rewritten so it merges all artifacts of the matrix builds.

@Kleidukos Kleidukos merged commit 6ec16e1 into Kleidukos:main Sep 1, 2024
15 checks passed
@mmhat mmhat deleted the wrapper branch September 1, 2024 10:53
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.

2 participants