Use of System.lineSeparator() prevents development on heterogenous systems #508
Comments
|
This might be a duplicate of #458? |
|
It's definitely related, but #458 relates more to the rule itself as key. Our rules are simple one-liners so the key isn't a problem, rather it's the violations themselves (which seem to be often multiline descriptions). |
|
Hmm, there is definitely some escaping / replacing of Windows linebreaks in place there, but maybe it's not enough 🤔 Do you have by any chance a minimal sample of a problematic scenario that works on Windows, but does not on Linux? (or vice versa, but since I'm using Linux myself it would be easier to reproduce, if I have a sample that works on Windows, but not on Linux 😉) I.e. the integration test To solve the (see https://www.archunit.org/userguide/html/000_Index.html#_overriding_configuration) Depending on your build tool you can configure the test task to pass this system property. |
|
Thanks for the quick response. I wrote a little test Maven project that runs in a similar way to our real project: https://github.com/gopf/archunit-test The store was frozen on my Mac, and the test runs fine there. If I run mvn clean test on a Windows machine it fails, though. In this case I fix up the archunit.properties with a Maven filter so the project basedir is used. That works pretty well. |
|
To make it easier, I checked in the store generated on Windows. This succeeds on Windows but fails on the Mac. |
|
Thanks a lot!! I'll use that sample to see how to fix it 😃 And try to fix #458 on the way, since that goes very much in the same direction. |
This addresses two problems: 1) if the rule description itself had different line separators than it was originally frozen with, `TextBasedViolationStore` would report `contains(rule) == false` and thus the result would be stored as new and the rule checking would succeed 2) if the single lines of the violation descriptions had a different line break than they were originally frozen with, these violations would all be removed as "solved" and the same violations with the different line break would be reported as new. I think unfortunately the API is a little strange, i.e. that `event.getDescriptionLines()` would return lines that contain a line break (since what is the use to talk about a list of lines if the lines can contain line breaks and are thus multiple lines in themselves). But unfortunately this is not so easy to change, since the counting of violations is based on this at the moment. For example, that one long cycle report `cycle detected ... \n dependencies of ... \n ...` is counted as a single "description line". To change this we would have to adjust this mechanism and then optimally make it illegal to return strings with line breaks from `event.getDescriptionLines()`. But extending `ConditionEvent` and thus implementing this method is public API, so this might be a breaking change again. Altogether I decided to just fix this within `FreezingArchRule` for now and see, if there will be further problems with this in other areas. To solve 1) I have adjusted `TextBasedViolationStore` to clean up line breaks from property names on save and load. I decided to add it to the file reading direction as well to a) also fix it for stores that have already saved Windows line breaks and b) support manual tinkering adding Windows line breaks in the future. Otherwise just storing them exclusively with `\n` as line separator would have probably been good enough. It would be better to solve 1) in a store independent way, so other store implementations would not have to deal with this problem, but unfortunately I could not find any way to do this, as long as the API is `contains(ArchRule)` and `store(ArchRule, ...)`. We could maybe always have converted these rules via `contains(rule.as(ensureUnixLineBreaks(rule.getDescription())))`, but that also felt a little shady, and might be surprising in a custom store implementation to get `rule.as(..)` instead of the rule directly. Problem 2) I have addressed within `FreezingArchRule` to make it independent of the concrete store implementation. I decided to put adapters on both "ends" of the rule, i.e. on the `EvaluationResult` producing the violations "lines" on the one side, and the violation store where we read violations from on the other side. That way comparisons should only happen between Unix line separators and violations should always be stored with Unix line breaks within the store. Resolves: #458 Resolves: #508
This addresses two problems: 1) if the rule description itself had different line separators than it was originally frozen with, `TextBasedViolationStore` would report `contains(rule) == false` and thus the result would be stored as new and the rule checking would succeed 2) if the single lines of the violation descriptions had a different line break than they were originally frozen with, these violations would all be removed as "solved" and the same violations with the different line break would be reported as new. I think unfortunately the API is a little strange, i.e. that `event.getDescriptionLines()` would return lines that contain a line break (since what is the use to talk about a list of lines if the lines can contain line breaks and are thus multiple lines in themselves). But unfortunately this is not so easy to change, since the counting of violations is based on this at the moment. For example, that one long cycle report `cycle detected ... \n dependencies of ... \n ...` is counted as a single "description line". To change this we would have to adjust this mechanism and then optimally make it illegal to return strings with line breaks from `event.getDescriptionLines()`. But extending `ConditionEvent` and thus implementing this method is public API, so this might be a breaking change again. Altogether I decided to just fix this within `FreezingArchRule` for now and see, if there will be further problems with this in other areas. To solve 1) I have adjusted `TextBasedViolationStore` to clean up line breaks from property names on save and load. I decided to add it to the file reading direction as well to a) also fix it for stores that have already saved Windows line breaks and b) support manual tinkering adding Windows line breaks in the future. Otherwise just storing them exclusively with `\n` as line separator would have probably been good enough. It would be better to solve 1) in a store independent way, so other store implementations would not have to deal with this problem, but unfortunately I could not find any way to do this, as long as the API is `contains(ArchRule)` and `store(ArchRule, ...)`. We could maybe always have converted these rules via `contains(rule.as(ensureUnixLineBreaks(rule.getDescription())))`, but that also felt a little shady, and might be surprising in a custom store implementation to get `rule.as(..)` instead of the rule directly. Problem 2) I have addressed within `FreezingArchRule` to make it independent of the concrete store implementation. I decided to put adapters on both "ends" of the rule, i.e. on the `EvaluationResult` producing the violations "lines" on the one side, and the violation store where we read violations from on the other side. That way comparisons should only happen between Unix line separators and violations should always be stored with Unix line breaks within the store. Resolves: #458 Resolves: #508
We have just started using ArchUnit in a small way, and have existing violations that we need to freeze.
Storing the currently frozen rules in the project along with the source seems to be the right thing to do. Getting that to work is a little bit tricky as I couldn't find a
freeze.store.default.pathsetting for this use case that works both locally and in the automated build, but I have a tolerable workaround (copy with variable expansion from Ant, use variable setting in the local run target to override).Unfortunately we have some developers that use Windows and some that use Macs, and the stored violations can't be shared between these two groups as the line endings vary. This is a blocker, and means that we can't really use ArchUnit.
The text was updated successfully, but these errors were encountered: