setup-dlang icon indicating copy to clipboard operation
setup-dlang copied to clipboard

Refactor code and implement more version formats for dmd and ldc and support gdc

Open the-horo opened this issue 1 year ago • 9 comments

A pretty big change to the code base with some fixes + enhancements.

Fixes:

  • https://github.com/dlang-community/setup-dlang/issues/50
  • https://github.com/dlang-community/setup-dlang/issues/40

Also unreported but dub: latest is failing due to a separate reason than https://github.com/dlang-community/setup-dlang/issues/64. It has been fixed as well.

CI has been modified to test all of the new ways a compiler can be specified and macos-latest has been downgraded to macos-13 due to aarch64 incompatibility of dmd and dub.

The ways a compiler can be specified are described in the code but, as a summary:

  • dmd|dmd-beta|dmd-latest|ldc|ldc-beta|ldc-latest
  • dmd-2.108.0|dmd-2.108.0-beta.1
  • ldc-1.35.0|ldc-1.36.0-beta1
  • dmd-2.108|ldc-1.34
  • dmd^3|ldc^3
  • dmd-master|ldc-master

The point of these changes is to make extending the action simpler and I have planned to implement basic support for gdc (only on linux and using apt, like https://github.com/dlang-community/setup-dlang/issues/35#issuecomment-2085441898 asked) but I want to put this out here to get it reviewed in the meantime.

Note that some of the macos jobs are failing due to https://issues.dlang.org/show_bug.cgi?id=24137. It has been fixed but it requires >=dmd-2.107.1

the-horo avatar May 05 '24 11:05 the-horo

I've implemented the support for gdc and gdmd.

The commits are currently kinda squished with a lot of fixes + enhancements intertwined, perhaps I should try to space them out more.

A summary of changes in the second commit:

  • support gdc(-version)? and gdmd(-version)? for compiler. Only on linux, fixes https://github.com/dlang-community/setup-dlang/issues/35
  • go back to dub latest in CI
  • and another step in the dub testing job that uses that dub instead of the one that came with the compiler to build and run the hello.d file.
  • the action takes in another optional argument, gdmd_sha. It is meant to be used to replace the gdmd version in the ubunty repos with something more up-to-date taken straight from https://github.com/D-Programming-GDC/gdmd. I wanted to implement this because there has been a recent commit that fixes -vcolumns which is quite relevant because dub adds that flag by default. I think that having every compiler be usable with dub is quite important and I didn't find another solution outside of letting users specify their needed version. You will notice that dub is still not happy with gdmd even though the version used corresponds to master. This is because of another bug related to the processing of @file arguments to read command line options from a file on disk.

the-horo avatar May 05 '24 23:05 the-horo

awesome, thanks for taking this on and giving us a much needed refactoring! As for the failing CI tests: it kinda looks like gdmd is missing the application arguments when it's run by dub. Maybe try running with dub -v to see what it's calling and try to manually call the command to see how it behaves and if it's flags that are breaking it or if it's missing the arguments altogether.

Do you need anything else?

WebFreak001 avatar May 06 '24 08:05 WebFreak001

I'd also suggest keeping osx on latest, and adding dmd + osx to the exclude section in the actions matrix. With include you can add the earlier versions in as needed

WebFreak001 avatar May 06 '24 08:05 WebFreak001

As for the failing CI tests: it kinda looks like gdmd is missing the application arguments when it's run by dub

Yes, I know what's wrong. gdmd only picks the first line of the @file, that's why arguments start going missing. I can make a PR that fixes it sometime today.

I'd also suggest keeping osx on latest, and adding dmd + osx to the exclude section in the actions matrix. With include you can add the earlier versions in as needed

I've went ahead and added different jobs for each compiler in order not to have to add some many -include and -exclude to the matrix. Check if all the platforms you wanted are tested.

Do you need anything else?

There are 2 unexpectedly failing jobs. I think I broke something so I will have to fix it. Other than this I only want to update the README and I think that should be all.

the-horo avatar May 06 '24 10:05 the-horo

if you specify only few things on the exclude/include matrix stuff, it should auto-generate the rest of the unspecified variables iirc

WebFreak001 avatar May 06 '24 10:05 WebFreak001

That's right, I didn't think of that. Yet I still don't think it's applicable. To use exclude it would be needed to say «exclude the configurations that have the os field set to macos-latest and the dc field match a pattern» which I don't see how one would do.

If I'm missing something obvious feel free to point it out explicitly but I don't see how, in the original code, one would be able to generate a matrix that runs on macos-13 all the dmd configurations and on macos-latest and macos-13 all the ldc configurations, without specifying, one by one, all the ldc configurations to be included on macos-latest or all the dmd configurations to be excluded from macos-latest.

the-horo avatar May 06 '24 12:05 the-horo

I've made all the changes that I wanted and, hopefully, I didn't miss any mistakes/typoes/bad rebases.

the-horo avatar May 06 '24 15:05 the-horo

did you make the gdmd fix PR yet? we can use it to avoid the CI failures since I saw you added an option to specify which gdmd commit to pick

WebFreak001 avatar May 07 '24 11:05 WebFreak001

did you make the gdmd fix PR yet? we can use it to avoid the CI failures since I saw you added an option to specify which gdmd commit to pick

Yes, I should have linked it here: https://github.com/D-Programming-GDC/gdmd/pull/16. I don't think we'll be able to use it until it's merged. I've implemented being able to pick a commit from the gdmd repo but I don't think PRs qualify, i.e. the commit with the fix belongs to my fork, not the upstream repo.

the-horo avatar May 07 '24 12:05 the-horo

I've had to fix an issue in which I was using "" instead of `` in a string but CI is all good now

the-horo avatar May 30 '24 05:05 the-horo

Has this changed the DC environment variable to an absolute path, whereas it used to be a filename only previously? This appears to have broken dub CI.

kinke avatar May 31 '24 15:05 kinke

as long as it's not updated to v2 this should not auto deploy

WebFreak001 avatar May 31 '24 15:05 WebFreak001

I think the branch names might have been conflicting with the version resolution, I renamed the dev branch to main now and renamed the old branch to old

WebFreak001 avatar May 31 '24 15:05 WebFreak001

@kinke can you create a test case for what broke on DUB and what you need fixed? Otherwise it might regress easily again

WebFreak001 avatar May 31 '24 15:05 WebFreak001

Well I have much better stuff to do. I'm just stumbling from one problem to the next. ;)

Have you removed the v1 ref now altogether?!

kinke avatar May 31 '24 15:05 kinke

v1 was renamed to old (and I fixed the branch to be on the v1.4.0 commit again)

WebFreak001 avatar May 31 '24 15:05 WebFreak001

Well then you've just broken all @v1 usages, as we do in dub and certainly many other places: https://github.com/dlang/dub/actions/runs/9320838532/job/25658507321?pr=2910

kinke avatar May 31 '24 15:05 kinke

bleh that's annoying, then I'll just rename it back

WebFreak001 avatar May 31 '24 15:05 WebFreak001

That v1 is broken though, due to the DC var being absolute now. That's a big breaking change and must be reverted.

kinke avatar May 31 '24 15:05 kinke

v1 exists again and I reverted it to be 1.4.0 instead of having this PR merged. This PR is now only available on v2

WebFreak001 avatar May 31 '24 15:05 WebFreak001

Ah alright alright, that sounds much better, thanks!

kinke avatar May 31 '24 15:05 kinke

I've tried to write a proper changelog in https://github.com/dlang-community/setup-dlang/pull/82

the-horo avatar May 31 '24 19:05 the-horo