dlang-community / setup-dlang

Github action for dlang compiler setup
MIT License
46 stars 14 forks source link

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

Closed the-horo closed 6 months ago

the-horo commented 6 months ago

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

Fixes:

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:

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 commented 6 months ago

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:

WebFreak001 commented 6 months ago

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 commented 6 months ago

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

the-horo commented 6 months ago

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.

WebFreak001 commented 6 months ago

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

the-horo commented 6 months ago

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 commented 6 months ago

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

WebFreak001 commented 6 months ago

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

the-horo commented 6 months ago

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 commented 6 months ago

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

kinke commented 6 months ago

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.

WebFreak001 commented 6 months ago

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

WebFreak001 commented 6 months ago

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 commented 6 months ago

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

kinke commented 6 months ago

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?!

WebFreak001 commented 6 months ago

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

kinke commented 6 months ago

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

WebFreak001 commented 6 months ago

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

kinke commented 6 months ago

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

WebFreak001 commented 6 months ago

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

kinke commented 6 months ago

Ah alright alright, that sounds much better, thanks!

the-horo commented 5 months ago

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