001-full-msbuild-support #199

aabs · 2026-01-24T04:41:50Z

PR Checklist (Fifth Language Engineering Constitution)

Please confirm each item before requesting review.

Builds the full solution without cancellation using documented commands
Tests added/updated first and now passing locally (unit/parser/integration as applicable)
No manual edits under src/ast-generated/; included metamodel/template changes and regeneration steps
For grammar changes: visitor updates and parser tests included
CLI behavior/contracts documented; outputs are deterministic and scriptable
Complexity increases are justified in the description
Versioning considered (SemVer); migration notes provided for breakages
Docs updated (README.md, AGENTS.md references) where behavior or commands changed

Summary

What changed and why?
User-visible impact (CLI, AST, grammar, codegen)?
Alternatives considered?

Validation

Provide commands you ran locally (paste exact output snippets as needed):

# Required sequence
DOTNET_CLI_TELEMETRY_OPTOUT=1 dotnet --info
java -version

dotnet restore fifthlang.sln
# Regenerate when changing AST metamodel/templates
# dotnet run --project src/ast_generator/ast_generator.csproj -- --folder src/ast-generated

dotnet build fifthlang.sln --no-restore

dotnet test test/ast-tests/ast_tests.csproj --no-build
# Optionally all tests
# dotnet test fifthlang.sln --no-build

Screenshots/Logs (optional)

Breaking Changes (if any)

Impacted users/scenarios:
Migration steps:

Related Issues/Links

…ta models, and quickstart guide

…k validation - Updated README.md to reflect support for library output and project/package references. - Modified Sdk.props to include support for multiple target frameworks. - Enhanced Sdk.targets to validate target frameworks and handle project/package references. - Updated Compiler.cs to support output type specification (Exe or Library) and reference resolution. - Added CompilerOptions.cs to include output type as a parameter. - Updated Program.cs to handle output type and references in command-line options. - Created SupportedTargetFrameworks.props to define supported frameworks. - Added unit tests for incremental builds, output type handling, and reference resolution.

…handling and source file resolution feat: Add Microsoft.Net.Compilers.Toolset dependency to multiple lock files fix: Correct output method in hello.5th test file

…ration

…blishing instructions

Copilot

Pull request overview

This PR implements comprehensive MSBuild support for Fifth projects, enabling library compilation, project/package references, incremental builds, multi-targeting, and design-time metadata generation. The changes span the compiler CLI, MSBuild SDK targets, and test infrastructure.

Changes:

Enhanced compiler with library output support, reference resolution, and directory-based source discovery
Extended MSBuild SDK with validation targets for frameworks/references, incremental build support, and design-time manifest generation
Added comprehensive SDK test suite covering output types, reference resolution, and incremental builds
Packaged compiler as .NET tool (Fifth.Compiler.Tool) with automated NuGet publishing workflow

Reviewed changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 17 comments.

Show a summary per file

File	Description
src/compiler/compiler.csproj	Added tool packaging metadata (PackAsTool, ToolCommandName, PackageId, Version)
src/compiler/Program.cs	Added OutputType and reference options; directory expansion for source files
src/compiler/CompilerOptions.cs	Added OutputType and References parameters with validation
src/compiler/Compiler.cs	Implemented library output support and reference assembly resolution
src/Fifth.Sdk/Sdk/Sdk.props	Set TargetExt/TargetPath based on OutputType; added SupportedTargetFrameworks import
src/Fifth.Sdk/Sdk/Sdk.targets	Added validation targets, incremental build inputs/outputs, reference resolution, and design-time manifest generation
src/Fifth.Sdk/Sdk/SupportedTargetFrameworks.props	Defined net8.0 as supported framework
src/Fifth.Sdk/README.md	Documented library support, tool installation, and publishing workflow
test/fifth-sdk-tests/*	New test project with OutputTypeTests, ReferenceResolutionTests, IncrementalBuildTests
test/fifth-sdk-tests/hello.5th	Test fixture Fifth source file
.github/workflows/release.yml	Added SDK and compiler tool NuGet publishing steps
docs/Development/publish-compiler-sdk.md	Publishing guide for SDK and compiler tool
docs/Designs/5thproj-implementation-summary.md	Updated to reflect library support and new capabilities
specs/001-full-msbuild-support/*	Complete feature specification with requirements, contracts, and task breakdown
*/packages.lock.json	Updated references from "compiler" to "Fifth.Compiler.Tool"

Comments suppressed due to low confidence (1)

src/compiler/CompilerOptions.cs:77

The validation logic returns null on line 76 inside the conditional block for SourceFiles validation, but there is additional validation for Source below it that would be skipped. If SourceFiles is present but Source is also provided and invalid, that validation won't run. Consider restructuring to check all validation conditions or clarifying the precedence between SourceFiles and Source.

        if (SourceFiles != null && SourceFiles.Count > 0)
        {
            var missing = SourceFiles.FirstOrDefault(path => !File.Exists(path) && !Directory.Exists(path));
            if (!string.IsNullOrWhiteSpace(missing))
            {
                return $"Source path does not exist: {missing}";
            }

            return null;
        }

Copilot · 2026-01-24T04:50:34Z

src/compiler/CompilerOptions.cs

+        if (!string.IsNullOrWhiteSpace(OutputType)
+            && !OutputType.Equals("Exe", StringComparison.OrdinalIgnoreCase)
+            && !OutputType.Equals("Library", StringComparison.OrdinalIgnoreCase))
+        {
+            return "Output type must be Exe or Library";
+        }
+
+        if (Command == CompilerCommand.Run && OutputType.Equals("Library", StringComparison.OrdinalIgnoreCase))
+        {
+            return "Run command is not supported for Library output";
+        }


The new compiler options (OutputType and References) added to CompilerOptions and Program.cs lack direct unit tests. While they're tested indirectly through SDK tests, there should be unit tests for the validation logic (e.g., OutputType validation, References handling) in CompilerOptions.Validate() to ensure edge cases are covered. Consider adding tests in a compiler test suite that verify validation behavior for invalid output types, Run command with Library output, etc.

Copilot · 2026-01-24T04:50:34Z

src/compiler/Program.cs

+                    continue;
+                }
+
+                resolvedSourceFiles.Add(sourceEntry);


The directory expansion logic does not check if a source entry is neither a file nor a directory before adding it to resolvedSourceFiles. If a non-existent path that is not a directory is passed, it will be added to resolvedSourceFiles without validation at this point, potentially causing issues downstream. Consider validating that file paths exist before adding them to resolvedSourceFiles, or add an else clause that reports an error for paths that are neither files nor directories.

Suggested change

resolvedSourceFiles.Add(sourceEntry);

if (File.Exists(sourceEntry))

{

resolvedSourceFiles.Add(sourceEntry);

}

else

{

Console.Error.WriteLine($"Source path does not exist or is not a file or directory: {sourceEntry}");

}

Copilot · 2026-01-24T04:50:35Z

.github/workflows/release.yml

+        shell: bash
+        env:
+          NUGET_PUBLISH: ${{ secrets.NUGET_PUBLISH }}
+          NUGET_SOURCE: https://api.nuget.org/v3/index.json


The workflow uses hardcoded NuGet source URL "https://api.nuget.org/v3/index.json" instead of using a workflow variable or secret. While this is the standard NuGet.org URL, it reduces flexibility for testing against different feeds (like MyGet or Azure Artifacts). Consider parameterizing this or documenting that it's intentionally hardcoded for the official release.

Suggested change

NUGET_SOURCE: https://api.nuget.org/v3/index.json

NUGET_SOURCE: ${{ vars.NUGET_SOURCE }}

Copilot · 2026-01-24T04:50:35Z

test/fifth-sdk-tests/IncrementalBuildTests.cs

+</Project>
+""");
+
+        File.WriteAllText(Path.Combine(projectDir, "hello.5th"), "main() => std.print(1);");


The test file uses std.print(1) but the hello.5th file in the same test directory uses unqualified write(). This inconsistency suggests that either both should use qualified calls like IO.write() or std.print(), or the test fixtures need to be aligned. The codebase examples in test/ast-tests/CodeSamples/ consistently use qualified calls like IO.write().

Suggested change

File.WriteAllText(Path.Combine(projectDir, "hello.5th"), "main() => std.print(1);");

File.WriteAllText(Path.Combine(projectDir, "hello.5th"), "main() => IO.write(1);");

Copilot · 2026-01-24T04:50:35Z

src/compiler/Program.cs

+            var primarySource = resolvedSourceFiles.FirstOrDefault()
+                ?? sourceDirectories.FirstOrDefault()
+                ?? string.Empty;

            var options = new CompilerOptions(
                Command: compilerCommand,
                Source: primarySource,


When both sourceDirectories and resolvedSourceFiles are empty, primarySource will be assigned an empty string. This might bypass validation in CompilerOptions.Validate() if SourceManifest is also null, potentially leading to unclear error messages. Consider explicitly validating that at least one source is resolved before constructing CompilerOptions.

Suggested change

var primarySource = resolvedSourceFiles.FirstOrDefault()

?? sourceDirectories.FirstOrDefault()

?? string.Empty;

var options = new CompilerOptions(

Command: compilerCommand,

Source: primarySource,

if (resolvedSourceFiles.Count == 0 && sourceDirectories.Count == 0 && string.IsNullOrEmpty(sourceManifest))

{

Console.Error.WriteLine("ERROR: No source files or source directories specified, and no source manifest provided.");

exitCode = 1;

return;

}

var primarySource = resolvedSourceFiles.FirstOrDefault()

?? sourceDirectories.FirstOrDefault();

var options = new CompilerOptions(

Command: compilerCommand,

Source: primarySource ?? string.Empty,

Copilot · 2026-01-24T04:50:38Z

.github/workflows/release.yml

+      - name: Pack SDK and compiler tool
+        if: steps.dryrun.outputs.value != 'true'
+        shell: bash
+        run: |
+          set -euo pipefail
+          VERSION='${{ steps.publish_version.outputs.value }}'
+          dotnet pack src/Fifth.Sdk/Fifth.Sdk.csproj -c Release /p:Version="$VERSION"
+          dotnet pack src/compiler/compiler.csproj -c Release /p:Version="$VERSION"


The pack commands don't verify that the resulting .nupkg files were created successfully before the publish step. If the pack step produces warnings or errors that don't fail the shell command (due to set -euo pipefail potentially not catching all issues), the publish step might attempt to push non-existent files. Consider adding explicit file existence checks after packing and before publishing.

Copilot · 2026-01-24T04:50:38Z

test/fifth-sdk-tests/OutputTypeTests.cs

+    [Fact]
+    public void ExeOutputTypeSetsTargetExtAndPath()
+    {
+        var (project, collection) = LoadProjectWithOutputType("Exe");
+        try
+        {
+            project.GetPropertyValue("TargetExt").Should().Be(".exe");
+            project.GetPropertyValue("TargetPath").Should().EndWith(".exe");
+            project.GetPropertyValue("FifthOutputPath").Should().Be(project.GetPropertyValue("TargetPath"));
+        }
+        finally
+        {
+            collection.Dispose();
+        }
+    }
+
+    [Fact]
+    public void LibraryOutputTypeSetsTargetExtAndPath()
+    {
+        var (project, collection) = LoadProjectWithOutputType("Library");
+        try
+        {
+            project.GetPropertyValue("TargetExt").Should().Be(".dll");
+            project.GetPropertyValue("TargetPath").Should().EndWith(".dll");
+            project.GetPropertyValue("FifthOutputPath").Should().Be(project.GetPropertyValue("TargetPath"));
+        }
+        finally
+        {
+            collection.Dispose();
+        }
+    }


The tests verify that project properties are set correctly (TargetExt, TargetPath) but don't verify that an actual build produces the correct output artifact type. Consider adding an end-to-end test that builds a Library project and verifies the resulting .dll can be loaded as a library, and similarly for Exe outputs. The current tests only validate MSBuild property evaluation, not the compilation pipeline.

Copilot · 2026-01-24T04:50:38Z

test/fifth-sdk-tests/ReferenceResolutionTests.cs

+    [Fact]
+    public void MissingProjectReferenceFailsBuild()
+    {
+        var repoRoot = FindRepoRoot();
+        var projectDir = CreateTempProjectDirectory();
+        var projectPath = Path.Combine(projectDir, "MissingProjectRef.5thproj");
+        var missingProjectPath = Path.Combine(projectDir, "Missing.5thproj");
+
+        File.WriteAllText(projectPath, $"""
+<Project Sdk=\"Fifth.Sdk\">
+  <PropertyGroup>
+    <TargetFramework>net8.0</TargetFramework>
+  </PropertyGroup>
+  <ItemGroup>
+    <ProjectReference Include=\"{missingProjectPath}\" />
+  </ItemGroup>
+</Project>
+""");
+
+        var result = BuildTarget(repoRoot, projectPath, "ResolveFifthProjectReferences");
+        result.OverallResult.Should().Be(BuildResultCode.Failure);
+    }
+
+    [Fact]
+    public void PackageReferencesRequireAssetsFile()
+    {
+        var repoRoot = FindRepoRoot();
+        var projectDir = CreateTempProjectDirectory();
+        var projectPath = Path.Combine(projectDir, "MissingAssets.5thproj");
+
+        File.WriteAllText(projectPath, """
+<Project Sdk="Fifth.Sdk">
+  <PropertyGroup>
+    <TargetFramework>net8.0</TargetFramework>
+  </PropertyGroup>
+  <ItemGroup>
+    <PackageReference Include="Newtonsoft.Json" Version="13.0.3" />
+  </ItemGroup>
+</Project>
+""");
+
+        var result = BuildTarget(repoRoot, projectPath, "ValidatePackageReferences");
+        result.OverallResult.Should().Be(BuildResultCode.Failure);
+    }


The tests verify failure cases (missing project references, missing assets file) but don't include a positive test case that verifies successful resolution and compilation with valid project and package references. This is critical for User Story 2 acceptance criteria. Consider adding a test that creates a valid multi-project setup and verifies the build succeeds with correct reference resolution.

Copilot · 2026-01-24T04:50:38Z

test/fifth-sdk-tests/hello.5th

@@ -1,3 +1,3 @@
 main(): void {
-    std.print("Hello from Fifth SDK!");
+    write("Hello from Fifth SDK!");


The unqualified call to write will fail at compile time. The Fifth.System library provides IO.write(), not a global write() function. This should be IO.write("Hello from Fifth SDK!") to match the pattern used in other test files like test/ast-tests/CodeSamples/destructuring.5th which uses IO.write().

Copilot · 2026-01-24T04:50:39Z

src/Fifth.Sdk/Sdk/Sdk.targets

-    <Error Condition="'$(FifthCompilerPath)' == '' OR !Exists('$(FifthCompilerPath)')" 
-           Text="Fifth compiler not found. Please set FifthCompilerPath property or environment variable to the full path of compiler.dll, or ensure the compiler is built in the repository at src/compiler/bin/." />
+        <Error Condition="'$(FifthCompilerCommand)' == '' AND ('$(FifthCompilerPath)' == '' OR !Exists('$(FifthCompilerPath)'))" 
+          Text="Fifth compiler not found. Set FifthCompilerCommand to a tool (e.g., fifthc), or set FifthCompilerPath to the full path of compiler.dll, or build the compiler in the repository at src/compiler/bin/." />


The error message suggests "Set FifthCompilerCommand to a tool (e.g., fifthc)" but the actual tool name from the compiler.csproj is "fifthc" (line 6 of compiler.csproj shows ToolCommandName=fifthc). The example is correct, but it might be clearer to emphasize that users should install the tool first with dotnet tool install -g Fifth.Compiler.Tool.

Suggested change

Text="Fifth compiler not found. Set FifthCompilerCommand to a tool (e.g., fifthc), or set FifthCompilerPath to the full path of compiler.dll, or build the compiler in the repository at src/compiler/bin/." />

Text="Fifth compiler not found. Install the Fifth compiler CLI tool (fifthc) with 'dotnet tool install -g Fifth.Compiler.Tool', set FifthCompilerCommand to that tool (e.g., fifthc), set FifthCompilerPath to the full path of compiler.dll, or build the compiler in the repository at src/compiler/bin/." />

aabs added 7 commits January 24, 2026 13:40

feat: Add specification and quality checklist for Full MSBuild Support

686c625

feat: Implement full MSBuild support with detailed specifications, da…

08d215f

…ta models, and quickstart guide

feat: Allow custom output extensions for compilation based on user input

62b3578

feat: Update Sdk.targets and CompilerOptions to enhance build target …

ba89cbf

…handling and source file resolution feat: Add Microsoft.Net.Compilers.Toolset dependency to multiple lock files fix: Correct output method in hello.5th test file

feat: Add ComputeRunArguments target for enhanced run command configu…

eded638

…ration

feat: Enhance Fifth SDK and Compiler Tool with MSBuild support and pu…

d51d55b

…blishing instructions

Copilot AI review requested due to automatic review settings January 24, 2026 04:41

aabs merged commit 60eff40 into master Jan 24, 2026
7 checks passed

Copilot started reviewing on behalf of aabs January 24, 2026 04:42 View session

aabs deleted the 001-full-msbuild-support branch January 24, 2026 04:43

Copilot AI reviewed Jan 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

001-full-msbuild-support #199

001-full-msbuild-support #199

Uh oh!

aabs commented Jan 24, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	NUGET_SOURCE: https://api.nuget.org/v3/index.json
	NUGET_SOURCE: ${{ vars.NUGET_SOURCE }}

	File.WriteAllText(Path.Combine(projectDir, "hello.5th"), "main() => std.print(1);");
	File.WriteAllText(Path.Combine(projectDir, "hello.5th"), "main() => IO.write(1);");

	Text="Fifth compiler not found. Set FifthCompilerCommand to a tool (e.g., fifthc), or set FifthCompilerPath to the full path of compiler.dll, or build the compiler in the repository at src/compiler/bin/." />
	Text="Fifth compiler not found. Install the Fifth compiler CLI tool (fifthc) with 'dotnet tool install -g Fifth.Compiler.Tool', set FifthCompilerCommand to that tool (e.g., fifthc), set FifthCompilerPath to the full path of compiler.dll, or build the compiler in the repository at src/compiler/bin/." />

001-full-msbuild-support #199

001-full-msbuild-support #199

Uh oh!

Conversation

aabs commented Jan 24, 2026

PR Checklist (Fifth Language Engineering Constitution)

Summary

Validation

Screenshots/Logs (optional)

Breaking Changes (if any)

Related Issues/Links

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants