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

GH2400: Add globber pattern support to the #load directive #2405

Merged
merged 4 commits into from
Mar 24, 2019

Conversation

gitfool
Copy link
Contributor

@gitfool gitfool commented Dec 20, 2018

Fixes #2400. @patriksvensson almost there; a couple of the existing unit tests are failing though:

In both cases the globber is processing absolute paths that don't have any globber patterns in them and returning no files. Any ideas how to handle that?

@gitfool gitfool changed the title [WIP] Add globber pattern support to the #load directive [WIP] GH2400: Add globber pattern support to the #load directive Dec 20, 2018
@gitfool gitfool force-pushed the gh2400 branch 3 times, most recently from 94877b6 to a8eba21 Compare January 8, 2019 23:26
@gitfool
Copy link
Contributor Author

gitfool commented Mar 18, 2019

@devlead @patriksvensson I had some time to dig into this; found and fixed the issue with the globber not working as expected with unix root paths. Also fixed some Windows tests.

I force pushed the changes, however the Azure Pipelines builds seem to be failing with an unrelated error in the integration tests:

System.PlatformNotSupportedException: Operation is not supported on this platform.
   at Microsoft.CSharp.CSharpCodeGenerator.FromFileBatch(CompilerParameters options, String[] fileNames)
   at Microsoft.CSharp.CSharpCodeGenerator.System.CodeDom.Compiler.ICodeCompiler.CompileAssemblyFromDomBatch(CompilerParameters options, CodeCompileUnit[] ea)
   at Mono.TextTemplating.TemplatingEngine.CompileCode(IEnumerable`1 references, TemplateSettings settings, CodeCompileUnit ccu) in /Users/mikayla/code/t4/Mono.TextTemplating/Mono.TextTemplating/TemplatingEngine.cs:line 271
   at Mono.TextTemplating.TemplatingEngine.CompileTemplateInternal(ParsedTemplate pt, String content, ITextTemplatingEngineHost host) in /Users/mikayla/code/t4/Mono.TextTemplating/Mono.TextTemplating/TemplatingEngine.cs:line 175
   at Mono.TextTemplating.TemplatingEngine.CompileTemplate(ParsedTemplate pt, String content, ITextTemplatingEngineHost host) in /Users/mikayla/code/t4/Mono.TextTemplating/Mono.TextTemplating/TemplatingEngine.cs:line 149
   at Mono.TextTemplating.ToolTemplateGenerator.ProcessTemplate(ParsedTemplate pt, String inputFile, String inputContent, String& outputFile) in /Users/mikayla/code/t4/dotnet-t4/ToolTemplateGenerator.cs:line 59
   at Mono.TextTemplating.TextTransform.MainInternal(String[] args) in /Users/mikayla/code/t4/dotnet-t4/TextTransform.cs:line 194
   at Mono.TextTemplating.TextTransform.Main(String[] args) in /Users/mikayla/code/t4/dotnet-t4/TextTransform.cs:line 42
An error occurred when executing task 'Cake.Common.Tools.TextTransform.TextTransformAliases.TransformTemplate.Properties'.

... and I have the same issue when running the integration tests locally. 🤔

@gitfool gitfool changed the title [WIP] GH2400: Add globber pattern support to the #load directive GH2400: Add globber pattern support to the #load directive Mar 18, 2019
@patriksvensson
Copy link
Member

@gitfool The T4 issue have been resolved, but I need to investigate the failing tests a bit. I will try to investigate this as soon as I have some spare time.

@gitfool
Copy link
Contributor Author

gitfool commented Mar 21, 2019

@patriksvensson re the unit tests that are now failing on *nix, I changed them from WindowsTheory to Theory because I thought they should now work on all platforms after I changed ScriptAnalyzerFixture ctor to create a Unix environment when specified.

I could change the attribute back but I'm still curious why they wouldn't work as the file system should be virtualized for the test.

@gitfool
Copy link
Contributor Author

gitfool commented Mar 21, 2019

I would guess the problem is coming from:

    if (_isWindows)
    {
        // Windows drive?
        if (length >= 2 && path[1] == ':')
        {
            return true;
        }
    }

i.e. PathHelper does not honor the fake environment.

@gitfool
Copy link
Contributor Author

gitfool commented Mar 22, 2019

@patriksvensson I'm keen to get this PR into the next release, and since the issue with the tests is due to a pre-existing deficiency in the fake test environment, I simply reverted the Windows tests to use WindowsTheory, as before.

@gitfool
Copy link
Contributor Author

gitfool commented Mar 22, 2019

Rebased and squashed the last revert commit for a cleaner merge.

Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Just about to board a plan so will take this for a spin later.

Could you add a integration test for this ( https://github.com/cake-build/cake/blob/develop/tests/integration/Cake.Core/Scripting/LoadDirective.cake ), preferably with an throw in a file that shouldn't be loaded by globber.

@gitfool
Copy link
Contributor Author

gitfool commented Mar 24, 2019

@devlead I added an integration test that adds a task dependency if loaded. One of the tasks throws an exception, but this does not happen since the globber pattern excludes it, while still loading the other task.

Note: I also tweaked the check for .cake file extension to be earlier so that a warning is logged if no scripts are found after filtering out any files without such an extension. I added some unit tests for this case too.

@devlead devlead merged commit b1b59c5 into cake-build:develop Mar 24, 2019
@devlead
Copy link
Member

devlead commented Mar 24, 2019

@gitfool your changes have been merged, thanks for your contribution 👍

@gitfool gitfool deleted the gh2400 branch March 24, 2019 05:31
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.

5 participants