Fixed #96: reserved template manifest properties are preserved #97
Conversation
ghost
assigned
| { | ||
| /// <summary> | ||
| /// Reads a valid v1.0 Jar Manifest. | ||
| /// See http://docs.oracle.com/javase/6/docs/technotes/guides/jar/jar.html#JAR%20Manifest |
There was a problem hiding this comment.
Simple reader to extract key-value pairs from an existing manifest file.
Required so we can copy single- and multi-line properties from the manifest in the template jar.
| private const string RelativeConfigurationResourcePath = "org\\sonar\\plugins\\roslynsdk\\configuration.xml"; | ||
| private const string RelativeRulesXmlResourcePath = "org\\sonar\\plugins\\roslynsdk\\rules.xml"; | ||
| private const string RelativeConfigurationResourcePath = "org/sonar/plugins/roslynsdk/configuration.xml"; | ||
| private const string RelativeRulesXmlResourcePath = "org/sonar/plugins/roslynsdk/rules.xml"; |
There was a problem hiding this comment.
The relative path is used for both file paths and in the archive entry names. It has to be / for the archive entries, but can be either for the file paths so it's simpler just to to use /.
There was a problem hiding this comment.
Maybe that's worth having this feedback directly in the code so we don't change it on the future (forgetting why this was done). Besides, maybe having a test on this (changing from private to internal) might help.
There was a problem hiding this comment.
I'll add a comment but I don't think it's worth adding further tests. The only one where it really matters is for the ManifestResource path, and there are about a dozen tests that already fail if that is wrong. The other strings are passed to the ArchiveUpdater.AddFile method, which is defensive and replaces the backslashes with forward slashes anyway.
| { | ||
| // This property must appear first in the manifest. | ||
| // See http://docs.oracle.com/javase/6/docs/technotes/guides/jar/jar.html#JAR%20Manifest | ||
| jarManifestBuilder.SetProperty("Sonar-Version", "4.5.2"); |
There was a problem hiding this comment.
This was the bug - the SDK was hard-coding values that should have been copied from the template jar.
| reader.FindValue("Sonar-Version").Should().Be("6.7"); | ||
| reader.FindValue("Plugin-Class").Should().Be("org.sonar.plugins.roslynsdk.RoslynSdkGeneratedPlugin"); | ||
| reader.FindValue("SonarLint-Supported").Should().Be("false"); | ||
| reader.FindValue("Plugin-Dependencies").Should().Be("META-INF/lib/jsr305-1.3.9.jar META-INF/lib/commons-io-2.6.jar META-INF/lib/stax2-api-3.1.4.jar META-INF/lib/staxmate-2.0.1.jar META-INF/lib/stax-api-1.0.1.jar"); |
There was a problem hiding this comment.
Regression test for the bug
PluginGenerator/JarManifestReader.cs
Outdated
| * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| */ | ||
|
|
||
|
|
| throw new ArgumentNullException(nameof(manifestText)); | ||
| } | ||
|
|
||
| kvps = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
[minor] This could be done inline with the declaration.
There was a problem hiding this comment.
It could, but if there is a constructor I prefer to initialize it there - less jumping around when debugging.
PluginGenerator/JarManifestReader.cs
Outdated
| var lines = joinedText.Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries); | ||
|
|
||
| // Every line should now be a key-value pair | ||
| foreach(var line in lines) |
There was a problem hiding this comment.
[cosmetic] Missing space after foreach
PluginGenerator/JarManifestReader.cs
Outdated
| // The simplest way to rejoin the lines is just to replace all (EOL + space) with EOL | ||
|
|
||
| var joinedText = manifestText.Replace("\r\n ", string.Empty); | ||
| var lines = joinedText.Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries); |
There was a problem hiding this comment.
I am not sure to understand the logic here. You have replaced all \r\n by string.Empty and then want to split on \r\n so you are not going to split anything.
There was a problem hiding this comment.
See the comment in line 48. It's replacing \r\n + space first i.e. any continuation lines will have been joined to a single line.
| "; | ||
| // Act | ||
| var jarReader = new JarManifestReader(text); | ||
|
|
| // multiple lines, with the continuation line starting with a single space. | ||
| // The simplest way to rejoin the lines is just to replace all (EOL + space) with EOL | ||
| var joinedText = manifestText.Replace("\r\n ", string.Empty); | ||
| var lines = joinedText.Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries); |
There was a problem hiding this comment.
Don't know why my previous comment was lost so I am redoing it:
This seems weird as you replace windows EOL by empty string and then on the replaced string you try to split on windows EOL.
There was a problem hiding this comment.
I deleted a line, which might by why your comment and my reply were lost.
See the comment in line 44: it's removing EOL + space to re-join all of the continuation lines together. Any EOL that are not followed by a space will still be there.
There was a problem hiding this comment.
OOOOOOOOH I didn't see the space after the \r\n... sorry for that.
| private const string RelativeConfigurationResourcePath = "org\\sonar\\plugins\\roslynsdk\\configuration.xml"; | ||
| private const string RelativeRulesXmlResourcePath = "org\\sonar\\plugins\\roslynsdk\\rules.xml"; | ||
| private const string RelativeConfigurationResourcePath = "org/sonar/plugins/roslynsdk/configuration.xml"; | ||
| private const string RelativeRulesXmlResourcePath = "org/sonar/plugins/roslynsdk/rules.xml"; |
There was a problem hiding this comment.
Maybe that's worth having this feedback directly in the code so we don't change it on the future (forgetting why this was done). Besides, maybe having a test on this (changing from private to internal) might help.
No description provided.