v2 improvements and fixes #83

the-horo · 2024-07-12T16:12:16Z

Addresses:

As a summary of changes:

additionally to DC to action will set DMD to point to the dmd wrapper of the compiler
the compiler: gdmd input has been removed, gdmd is now always installed alongside gdc
dmd is now usable on arm64 macos
all the dependencies has been updated.
the target output has become es2022 from es2017
recent releases of dub will now use arm64 binaries on arm64 macos
added unittests written in typescript that can be run with npm test (on any platform).
The current workflow when contributing can be:
```
# make changes
npm test
npm run build
git commit && git push # and wait for CI
```
fix the verify-d-compiler action which incorrectly tried to run a shared library

Geod24

This is fantastic work, thank you! Main concern is gdmd_sha should have an option for latest. Haven't fully reviewed the tests as it's pretty massive, but overall looks very good, thanks!

One note, README says:

Changes from v2

Should it be Changes from v1 or are we going for v3 so soon?

Geod24 · 2024-07-12T18:05:04Z

src/index.ts

@@ -0,0 +1,2 @@
+import { run } from './main'
+run()


I think you should change the build script as a consequence ? First commit.

Yeah, I think that needs to be changed. I've only done actual CI on the action since the dmd arm64 commit so, before that, it's possible that the action became broken.

src/main.ts

Geod24 · 2024-07-12T18:08:36Z

src/d.ts

@@ -333,8 +333,14 @@ export class DMD extends Compiler {
 	    case "win32":
 		url += '.windows'
 		url += minor < 69 ? '.zip' : '.7z'
-		binPath = '\\dmd2\\windows\\bin'
-		libPaths = [ "\\dmd2\\windows\\bin64" ]
+		// Since 2.091.0-beta.1 there are 64 bit binraries,


Typo: binaries (3rd commit).

Geod24 · 2024-07-13T08:55:57Z

__tests__/action_input.test.ts

+    function mockCompiler(compiler: string) {
+	jest.spyOn(core, 'getInput').mockImplementation((key) => {
+	    if (key == "compiler")
+		return compiler


Indentation seems to be off in a few place in this file, but this is definitely the worst offender.

I guess Github displays stuff with tabs being 4 spaces so that's why it looks off. My emacs is doing 4 characters indents and collapsing 8 spaces into a single tab.

It seems reasonable to add an .editorconfig file and enforce some sane defaults. Does:

indent_style = tab indent_size = 4

sound ok for the project?

PS in the conversation tab the indentation looks fine (4 spaces for each line) but in the files tab it looks messed up.

I think indent_style should be spaces. But we can keep it for another PR, let's not have indentation block us from getting those new features in.

Geod24 · 2024-07-13T09:01:42Z

README.md

@@ -179,7 +174,8 @@ Github [generates](https://docs.github.com/en/actions/security-guides/automatic-

 ### gdmd_sha

-In case the `gdmd` script in the ubuntu repositories is too old you can specify a commit sha in https://github.com/D-Programming-GDC/gdmd and this action will use that to download a git version of the script.
+The gdmd script is downloaded straight from the [upstream](https://github.com/D-Programming-GDC/gdmd) repository.


We should have latest as a symbolic value, otherwise updates to GDMD will requires updates here (and version bump).

Due to how to URL is calculated putting in master means that the script is downloaded from https://raw.githubusercontent.com/D-Programming-GDC/gdmd/master/dmd-script which is the latest version. No need to add any code.

Is it fine if it's left as master or do we make it latest for consistency. I'm fine we both but the former doesn't require changing any code.

I think having latest for consistency and displaying this in example is better. This way if upstream decide to change their branch naming convention we can adapt our code without penalizing the user.

the-horo · 2024-07-13T11:48:08Z

This is fantastic work, thank you! Main concern is gdmd_sha should have an option for latest. Haven't fully reviewed the tests as it's pretty massive, but overall looks very good, thanks!

One note, README says:

Changes from v2

Should it be Changes from v1 or are we going for v3 so soon?

Depends on how we want to perceive the v2 branch. If we consider it as being a normal release then yes, the action should be bumped to v3 as there has been one breaking change:

using compiler: gdmd is no longer supported

This could be fixed by reallowing that input and making DC point to DMD to preserve the generated environment. Now that I consider, setting DMD on top of DC does sound like a breaking change since users may have been using that variable.

Keep in mind that I say this with the previous PR which I've made in mind that ended up breaking other projects' CI because of the changes.

If the v2 branch is perceived as v2-alpha, so a prerelease, then yeah, it's fine to keep it at v2. Or at least that how I think semver is supposed to work for actions

Move the action entry point into a separate file and split the code that handles the action's inputs and their default values to a separate function. This will make it possible to perform tests on the main action function. Signed-off-by: Andrei Horodniceanu <[email protected]>

Making members public will allow inspecting their values in tests. Signed-off-by: Andrei Horodniceanu <[email protected]>

Currently, on windows, the action incorrectly sets DC to point to the 32-bit variant of the executable. This has been fixed by using the 64-bit variant when it is knows to be available. On macos the LD_LIBRARY_PATH variable was set incorrectly (to `/dmd2/linux/lib`) this did not seem to affect the compiler as there aren't any shared libraries available for osx. Signed-off-by: Andrei Horodniceanu <[email protected]>

Add an error message for dmd being used on non-x86_64 and correct some other messages. Signed-off-by: Andrei Horodniceanu <[email protected]>

There is already code in the action that picks between dmd-latest and ldc-latest based on the CPU architecture when a D compiler is not specified. Having the default value in action.yml prevented this code from ever running. This leads to users receiving an error message and having to specify `dc: ldc` manually. Bug: dlang-community#78 Signed-off-by: Andrei Horodniceanu <[email protected]>

Currently all of the tests are written as a github workflow. This makes them very inconvenient to run as one would have to be triggering the online CI every times they make a change to the code. On top of this the tests can only verify the final product of the action, if changes are made somewhere deep in the source code and they break something it will not be immediately clear what is broken. For this reason I've written tests in plain typescript that are much easier to be run but still check all the platforms (linux, windows, and macos) on x86_64 and arm64. These tests are covering a big part of the source code, mostly the logic bits in addition to mocking network responses for the code that contacts github.com or dlang.org to determine the versions that needs to be installed. This should make future work on the action way easier. This doesn't make the old tests obsolete as some parts of the code are not easily tested, like fetching release archives and properly extracting them. Add an action that will run the unittests on each system (linux, macos, windows) to assure that any person can run the tests on their system without failures. Signed-off-by: Andrei Horodniceanu <[email protected]>

Geod24 · 2024-07-13T16:59:00Z

Regarding v2, it's not an -alpha one unfortunately (https://github.com/dlang-community/setup-dlang/releases/tag/v2.0.0) but I don't think it would add much value to the users to bump to v3 so early.

the-horo · 2024-07-13T20:20:02Z

I've tried to address your issues (except for the indentation style) let me know if you're fine with the solutions.

I spotted that I accidentally removed the gdc CI tests when introducing $DMD but now their back

This action sets up DC to point to the compiler. It is not much more work to also set DMD to point to the dmd wrapper of the compiler. dmd is its own wrapper, ldmd2 comes by default with ldc2. The only one left out is gdc. The action allowed installing gdc, either alone or with gdmd and setting DC to point to one of them. This required the user to specify which what they wanted to set DC to. This is pointlessly complex as gdmd is just a script file that can be easily downloaded. The previous decision to do this separately was because of the installation method of the gdmd script, it defaulting to installing it through `apt`, which would likely lead to many more useless packages being installed. This is too much work for just a script file so it will now always be installed by fetching it from the upstream repo. See dlang-community#77 (comment) for a discussion around this topic. This has as consequences that the `gdmd` and `gdmd-<version>` inputs to `compiler` become invalid and that the `gdmd_sha` inputs becomes mandatory. To accommodate for the second issue the action now picks a default value for `gdmd_sha`. Signed-off-by: Andrei Horodniceanu <[email protected]> Add CI for gdc and gdmd Signed-off-by: Andrei Horodniceanu <[email protected]>

Signed-off-by: Andrei Horodniceanu <[email protected]>

Avoid the possibility of mistakenly running a previously generated hello executable by performing all the steps in separate directories. This has shown an issue with the `-shared` test, it never produced an executable so invoking the program was useless as it run the one that was generated in the previous step. For this reason in that test perform only compilation and no longer run any binary. Signed-off-by: Andrei Horodniceanu <[email protected]>

Remove the dependency on promisify-child-process by using the exec function provided by @actions/exec. Signed-off-by: Andrei Horodniceanu <[email protected]>

all macos images come with gpg preinstalled, see: https://github.com/actions/runner-images/tree/main/images/macos for the list of all preinstalled packages. Signed-off-by: Andrei Horodniceanu <[email protected]>

Signed-off-by: Andrei Horodniceanu <[email protected]>

Update all dependencies to their highest version available. Change the target option to es2022 from es2017 to match the current configuration of the upstream typescript action. These changes mean that --openssl-legacy-provider is no longer needed when building so the option has been dropped from the building step of the workflow. Signed-off-by: Andrei Horodniceanu <[email protected]>

Signed-off-by: Andrei Horodniceanu <[email protected]>

Clearing the github defined variables makes @actions/core print to stdout instead of writing to the files, which allows the tests to mock other platforms without the fear of paths breaking. Signed-off-by: Andrei Horodniceanu <[email protected]>

Since version 1.38.1 dub started publishing arm64 releases for macos in addition to their x86_64 releases. Signed-off-by: Andrei Horodniceanu <[email protected]>

the-horo · 2024-07-14T05:05:00Z

Resetting the GITHUB_ENV and GITHUB_PATH environment variables is a cleaner way to run the unittests during CI compared to importing internal functions.

Collapse all "from v2" and "from v1" changes into a single category and remove entries that document changes internal to the v2 release. Order the entries by their relevance, most relevant first. Signed-off-by: Andrei Horodniceanu <[email protected]>

Geod24 · 2024-07-14T10:13:40Z

@the-horo : Will try to review again tonight / tomorrow but I expect this can go in soon. Also invited you to the project.

the-horo · 2024-07-14T10:20:56Z

Thanks. Feel free to work on your pace as I'll have the time to fix stuff if you find anything broken.

Geod24 · 2024-07-18T08:02:48Z

Thanks a lot @the-horo ! The nice commit history and the amount of testing is greatly appreciated.

Geod24 reviewed Jul 13, 2024

View reviewed changes

the-horo added 6 commits July 13, 2024 14:50

src/d.ts: drop restrictive visibility specifiers on members

2563280

Making members public will allow inspecting their values in tests. Signed-off-by: Andrei Horodniceanu <[email protected]>

src/d.ts: fix some error messages

5416293

Add an error message for dmd being used on non-x86_64 and correct some other messages. Signed-off-by: Andrei Horodniceanu <[email protected]>

the-horo force-pushed the vtests branch 3 times, most recently from a5083f1 to 4c4acda Compare July 13, 2024 19:47

the-horo added 9 commits July 14, 2024 07:17

Allow dmd on macos arm64

c56aaa8

Signed-off-by: Andrei Horodniceanu <[email protected]>

src/gpg.ts: Use @actions/exec instead of promisify-child-process

15a7258

Remove the dependency on promisify-child-process by using the exec function provided by @actions/exec. Signed-off-by: Andrei Horodniceanu <[email protected]>

Don't try to install gpg on macos, it is already installed

9c0785a

all macos images come with gpg preinstalled, see: https://github.com/actions/runner-images/tree/main/images/macos for the list of all preinstalled packages. Signed-off-by: Andrei Horodniceanu <[email protected]>

Remove dependency on @actions/io as it is unused

0840768

Signed-off-by: Andrei Horodniceanu <[email protected]>

src/gpg.ts: Use core.toPosixPath instead of internal implementation

be56a18

Signed-off-by: Andrei Horodniceanu <[email protected]>

Set fail-fast to false in unittest action

74aa168

Signed-off-by: Andrei Horodniceanu <[email protected]>

the-horo force-pushed the vtests branch 2 times, most recently from 96d9ab1 to 817db3b Compare July 14, 2024 04:59

the-horo added 2 commits July 14, 2024 08:00

dub: Use arm64 binaries on arm64 macos when available

73502c9

Since version 1.38.1 dub started publishing arm64 releases for macos in addition to their x86_64 releases. Signed-off-by: Andrei Horodniceanu <[email protected]>

the-horo force-pushed the vtests branch from 817db3b to 30ede70 Compare July 14, 2024 05:01

README.md: Reword changelog entries

21245d7

Collapse all "from v2" and "from v1" changes into a single category and remove entries that document changes internal to the v2 release. Order the entries by their relevance, most relevant first. Signed-off-by: Andrei Horodniceanu <[email protected]>

the-horo force-pushed the vtests branch from 30ede70 to 21245d7 Compare July 14, 2024 06:47

Geod24 merged commit e5ca478 into dlang-community:v2 Jul 18, 2024
101 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2 improvements and fixes #83

v2 improvements and fixes #83

the-horo commented Jul 12, 2024

Geod24 left a comment

Geod24 Jul 12, 2024

the-horo Jul 13, 2024

Geod24 Jul 12, 2024

Geod24 Jul 13, 2024

the-horo Jul 13, 2024

Geod24 Jul 13, 2024

Geod24 Jul 13, 2024

the-horo Jul 13, 2024

Geod24 Jul 13, 2024

the-horo commented Jul 13, 2024

Changes from v2

Geod24 commented Jul 13, 2024

the-horo commented Jul 13, 2024

the-horo commented Jul 14, 2024

Geod24 commented Jul 14, 2024

the-horo commented Jul 14, 2024

Geod24 commented Jul 18, 2024

v2 improvements and fixes #83

v2 improvements and fixes #83

Conversation

the-horo commented Jul 12, 2024

Geod24 left a comment

Choose a reason for hiding this comment

Changes from v2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-horo commented Jul 13, 2024

Changes from v2

Geod24 commented Jul 13, 2024

the-horo commented Jul 13, 2024

the-horo commented Jul 14, 2024

Geod24 commented Jul 14, 2024

the-horo commented Jul 14, 2024

Geod24 commented Jul 18, 2024