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

Warn and skip code gen for duplicate aliases #2487

Closed
devlead opened this Issue Feb 24, 2019 · 17 comments

Comments

Projects
None yet
4 participants
@devlead
Copy link
Member

commented Feb 24, 2019

PR #2455 created a conflict with existing Cake.Incubator addin

- (2688,10): error CS0111: Type 'Submission#0' already defines a member called 'EnvironmentVariable' with the same parameter types

Repro script

#addin nuget:?package=Cake.Incubator&version=3.1.0

What we think we need to do here is track methods generated by code gen and skip & warn on any duplicates, prioritizing what's in Cake.Common.

Likely would need to add using statements to avoid errors with aliases being invoked on ICakeContext

@devlead devlead added the Bug label Feb 24, 2019

@devlead devlead added this to the v0.33.0 milestone Feb 24, 2019

@devlead

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

Did a quick PoC that did a quick n' dirty signature duplicate check of alias methods/properties, which solves already defines a member called.

For code gen it's possible to exclude individual aliases, issue is if aliases if executed as extension methods on ICakeContext, then only option is to exclude entire namespace, which is needed to solve ambiguous method calls.

Looking at Cake.Incubator addin that would mean

- Method "EnvironmentVariable" excluded from code generation and ill need to be fully qualified on ICakeContext.
- Method "Move" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "Move" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "GetMatchingFiles" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "GetFiles" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "ParseProject" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "ParseProject" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "GetOutputAssemblies" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "GetOutputAssemblies" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "GetSolutionAssemblies" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "GetSolutionAssemblies" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "GetProjectAssembly" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "GetProjectAssemblies" was included in code generation, but will need to be fully qualified on ICakeContext.
- Method "GetProjectAssemblies" was included in code generation, but will need to be fully qualified on ICakeContext.

Which would likely break many scripts, but in return it would at least be possible to fix scripts by using fully qualified names/using static for extension methods.

Example use string extensions

#addin nuget:?package=Cake.Incubator&version=3.1.0
using static Cake.Incubator.StringExtensions;

Information("true".EqualsIgnoreCase("True"));

Potentially we could generate extension method proxies to for non duplicate aliases to reduce number of scripts causing issues, but rather unsure this would be worth it. Feels like a possible rabbit hole of issues. Also most extension methods in Cake.Incubator aren't aliases, so wouldn't help much.

A more long term solution could be to update guidance for addins to not use just one namespace for all classes, but instead structure in multiple namespaces, reducing the impact if we exclude namespace.

I.e. if ProjectParserExtensions would've been in a Cake.Incubator.Solution.Project namespace and EnvironmentExtensions would've been in it's own extension, then the introduction of EnvironmentVariable in Cake.Common would only exclude EnvironmentExtensions.

Thoughts?

devlead added a commit to devlead/cake that referenced this issue Feb 24, 2019

PoC (cake-buildGH-2487) fix
Don't merge, just a quick PoC what parts affected by the issue,
it does so by a dupe check on method/property signarture,
if a colliede occurs it excludes that member from code gen,
it also excludes it's namespace from default imported namespaces.
For discusssion, thoughts see issue cake-build#2487
@devlead

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

Added #2488 just for reference as a draft yolo pr ;)

@gep13

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

@wwwlicious adding you to get some input on this as well.

@wwwlicious

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

@gep13 @devlead So I can certainly move things into namespaces per extension if that is what you are suggesting.

It would also be beneficial for me to know if duplicates are added into the core cake repos so I can deprecate them over the span of a few releases. No point it being in both.

It might be an opportune time for you to review what is in cake.incubator and cherry pick anything else you'd like in the core libs, that way we can co-ordinate removing them and setting min dependencies for upcoming releases.

there are a number of methods already marked as obsolete which can prob be removed for the next maj version, a couple of which were in your list above.

    <Cake.Incubator>\CustomProjectParserResult.cs  (2 usages found)
        Cake.Incubator  (2 usages found)
            CustomProjectParserResult  (2 usages found)
                (45: 10)  [Obsolete("Use OutputPaths instead for multi-targeting support")]
                (63: 10)  [Obsolete("Use TargetFrameworkVersions instead")]
    <Cake.Incubator>\DirectoryExtensions.cs  (1 usage found)
        Cake.Incubator  (1 usage found)
            DirectoryExtensions  (1 usage found)
                (29: 10)  [Obsolete("Use Cake.Common.IO.CopyDirectory instead")]
    <Cake.Incubator>\FileExtensions.cs  (2 usages found)
        Cake.Incubator  (2 usages found)
            FileExtensions  (2 usages found)
                (45: 10)  [Obsolete("Use Cake.Common.IO.MoveFile instead")]
                (66: 10)  [Obsolete("Use Cake.Common.IO.MoveFiles instead")]
    <Cake.Incubator>\NetFrameworkProjectProperties.cs  (1 usage found)
        Cake.Incubator  (1 usage found)
            NetFrameworkProjectProperties  (1 usage found)
                (23: 10)  [Obsolete("Use TargetFrameworkVersions insead")]
    <Cake.Incubator>\ProjectParserExtensions.cs  (2 usages found)
        Cake.Incubator  (2 usages found)
            ProjectParserExtensions  (2 usages found)
                (292: 10)  [Obsolete("Use GetAssemblyFilePaths instead for multi-targeting support")]
                (704: 10)  [Obsolete("Use GetProjectAssemblies instead which includes support for multi-targeting projects")]
@devlead

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@gep13 @devlead So I can certainly move things into namespaces per extension if that is what you are suggesting.

That would be great 👍 , especially EnvironmentVariable would be good to get out in it's own namespace as that'll otherwise cause issues in the next release of Cake.

If we lift things from "incubation" to "official" having a few namespaces will make that a more seamless iterative process.

@wwwlicious

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@gep13 can you take a look at this so I can get a new incubator release out?
https://ci.appveyor.com/project/cakecontrib/cake-incubator/branch/master

@gep13

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

@wwwlicious for that to work, you will need to bump the version of Cake that you are using here:

https://github.com/cake-contrib/Cake.Incubator/blob/develop/tools/packages.config#L3

As a minimum, you will need 0.32.0.

Hope that helps

@wwwlicious

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@gep13 tried updating in dev and master branch, running ClearCache target. Looks like appveyor still restoring cached copy. https://ci.appveyor.com/project/cakecontrib/cake-incubator/builds/22666972

@gep13

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

@wwwlicious hmm, very strange! I have just tried clearing the cache, and it also isn't working for me. I seem to remember @pascalberger having a similar issue the other day, and he asked a question on Twitter. Don't know if he got a response or not though.

Based on this configuration:

https://github.com/cake-contrib/Cake.Incubator/blob/develop/.appveyor.yml#L27

Can you try making a slight alteration to the setup.cake file, see if that can force it through?

@wwwlicious

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@gep13 Just removing caching from appveyor.

Version 4.0.0 should be available on nuget shortly.

@gep13

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

@wwwlicious said...
Just removing caching from appveyor.

That works too.

@devlead

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@wwwlicious with 4.0 impact looks better

- Namespace Cake.Incubator.EnvironmentExtensions excluded by code generation, affected methods:
-         Method "EnvironmentVariable" excluded from code generation and will need to be fully qualified on ICakeContext.

devlead added a commit to devlead/cake that referenced this issue Feb 26, 2019

devlead added a commit to devlead/cake that referenced this issue Feb 26, 2019

@wwwlicious

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

So it looks like this is breaking things for users. Is there a min cake version they will require?
cake-contrib/Cake.Incubator#99

@devlead

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

No new Cake version should be required.

If there's no aliases in a namespace it won't to be imported automatically and would need a using statement tool be found.

You can automatically import namespaces via attributes on an alias

[CakeNamespaceImport("Cake.Common.Tools.DotNetCore.Execute")]

devlead added a commit to devlead/cake that referenced this issue Feb 27, 2019

devlead added a commit to devlead/cake that referenced this issue Feb 27, 2019

devlead added a commit to devlead/cake that referenced this issue Feb 27, 2019

patriksvensson added a commit that referenced this issue Feb 27, 2019

Merge pull request #2488 from devlead/feature/gh-2487
GH2487: Add alias dupe check on method/property signarture
@gitfool

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

I've just caught up with the changes and everything works together, but there are still the potential issues that my implementation is not exactly the same as what is in Cake.Incubator since I didn't know about its implementation and I based it on ArgumentAliases:

I assign the default value if the environment variable is null:

EnviromentAliases.cs#L71 (like ArgumentAliases.cs#L117):

return value == null ? defaultValue : Convert<T>(value);

Whereas Cake.Incubator assigns the default value if the environment variable is null or empty:

EnvironmentExtensions.cs#L71:

return string.IsNullOrEmpty(value) ? defaultValue : Convert<T>(value);

Also, I did not implement the other method that does not take a default value and throws:

EnvironmentExtensions.cs#L36:

public static T EnvironmentVariable<T>(this ICakeContext context, string variable)

There is a similar method in ArgumentAliases.cs#L72, but again it only checks for null, not null or empty, and it didn't seem like a good idea to throw an exception if an environment variable is missing, so I didn't bring it over.

So in summary, I was implementing this in a way that was consistent with ArgumentAliases, also taking into account usage for environment variables which tends to allow them to be missing; however this is a breaking change when compared to the current Cake.Incubator implementation.

@gitfool

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

@devlead looks like you’re preparing for a release soon. Do you want me to do anything about the issues above, or leave it as is?

@devlead

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@gitfool I'll say leave as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.