-
Notifications
You must be signed in to change notification settings - Fork 228
Change the generated names for resources #1486
Conversation
PS: tested on mono and it works |
/// </summary> | ||
/// <param name="name"></param> | ||
/// <returns></returns> | ||
private static string MakeValidEverettIdentifier(string name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what "Everett" means here, but it might be good to either: a) put a comment above explaining what "Everett" means here or b) rename the methods to the actual VS version name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's code copied from MsBuild, even the comments (I put a reference above)
Looks pretty good so far. A few comments. I'll rebase my PR on it, it shouldn't be too bad. I expect that ResourceDescriptor will move to |
@@ -121,7 +121,24 @@ public class RoslynCompiler | |||
|
|||
compilation = ApplyVersionInfo(compilation, project, parseOptions); | |||
|
|||
var compilationContext = new CompilationContext(compilation, project, target.TargetFramework, target.Configuration); | |||
CompositeResourceProvider resourceProvider = new CompositeResourceProvider(new IResourceProvider[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the code shouldn't know anything about resource providers.
Updated to address the comments from @anurse and @davidfowl |
@@ -13,7 +13,7 @@ public class ProjectFilesCollection | |||
public static readonly string[] DefaultBundleExcludePatterns = new[] { @"obj/**/*.*", @"bin/**/*.*", @"**/.*/**" }; | |||
public static readonly string[] DefaultPreprocessPatterns = new[] { @"compiler/preprocess/**/*.cs" }; | |||
public static readonly string[] DefaultSharedPatterns = new[] { @"compiler/shared/**/*.cs" }; | |||
public static readonly string[] DefaultResourcesPatterns = new[] { @"compiler/resources/**/*" }; | |||
public static readonly string[] DefaultResourcesPatterns = new[] { @"compiler/resources/**/*.resx" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't right. There should be 2 default resource patterns. This array should have:
public static readonly string[] DefaultResourcesPatterns = new[] { @"compiler/resources/**/*", "**/*.resx" };
@davidfowl updated |
Very nice. Now I'm going to pull it down and try it. |
Let me rebase it first |
1b9754d
to
61458d4
Compare
@davidfowl you can try now |
Can we get this in soon? @davidfowl are you good with it now? |
This will break all of the other e repertories. Do you have a change ready for them? |
Wow, repositories |
Actually, I don't think it will break in too many places. All I have to do is change the resource generator in Universe. But hold on because I found a bug. |
It works with the existing code because we generate the same names. In the second iteration I changed the name of the |
If this doesn't break then we did something wrong. |
Tried out the resource sharing scenario and that's still busted. It's a tricky one too. If you share resources with another project from a project.json based project, the resource names will be for the project.json based project instead of the project we're using. This makes the logical names wrong and it makes using those resources at runtime fail: ClassLibrary58 using System;
namespace ClassLibrary58
{
public class Program
{
public void Main()
{
Console.WriteLine(ClassLibrary1.Resource1.Another);
}
}
} {
"version": "1.0.0-*",
"commands": {
"ClassLibrary58": "ClassLibrary58"
},
"compile": [ "**/*.cs", "../../ClassLibrary1/**/*.cs" ],
"resource": [ "../../ClassLibrary1/**/*.resx" ],
"frameworks": {
"dnx451": {
"dependencies": {
}
}
}
} The generated code from Resource1: [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Advanced)]
internal static global::System.Resources.ResourceManager ResourceManager {
get {
if (object.ReferenceEquals(resourceMan, null)) {
global::System.Resources.ResourceManager temp = new global::System.Resources.ResourceManager("ClassLibrary1.Resource1", typeof(Resource1).GetTypeInfo().Assembly);
resourceMan = temp;
}
return resourceMan;
}
} This breaks because: This is the case where we need to be able to specify the project root for a resource name. {
"resource": [
{
"pattern": "../../ClassLibrary1/**/*.resx",
"logicalName": "ClassLibrary1"
}
],
} What do you think? |
It does look like we need to adjust that. Can I ask that we try to get this PR in ASAP and maybe file a bug for immediate follow-up for that issue? Is the base functionality broken or are we just talking about fixing a specific scenario? The compilation refactor is really itching to be pushed (so I can get new runtime compilation going and all that fun stuff) and it's a non-trivial rebase. I don't want to do that rebasing if this PR is going to be rebased (making me rerebase and thus reresad). |
Yes, I will get this PR merged soon and then start another one for the |
👍 Thanks much! |
6a1babe
to
c598240
Compare
Because of bootstrapping issues, I hacked the code to generate both old and new resource names. Therefore, this change should not break anything. I am going to merge it |
…roduced by MsBuild
c598240
to
6890623
Compare
Fixes #502 and #898
Please review @davidfowl @lodejard
Notice there is some code that I copied from MsBuild.
cc @anure you will have to rebase on top of this