Bug 453954 - [patch] Target editor line delimiters change to the system default when saving
Summary: [patch] Target editor line delimiters change to the system default when saving
Status: REOPENED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.5   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: PDE-UI-Inbox CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords:
Depends on:
Blocks: 515662
  Show dependency tree
 
Reported: 2014-12-02 21:04 EST by Marc-André Laperle CLA
Modified: 2023-07-24 02:03 EDT (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Marc-André Laperle CLA 2014-12-02 21:04:36 EST
Using Eclipse 3.5-I20141202-0800

1. Open an existing .target file. If on Windows, the files needs to have Unix line delimiters (\n). On Linux/Mac, the file needs to have Windows delimiters (\r\n).
2. Modify the file by adding a feature from a p2 repository, for example.
3. Save the file.

The file now has the system default line delimiters instead of using the existing delimiter from the original file.
Comment 1 Marc-André Laperle CLA 2014-12-02 21:07:36 EST
Patch:
https://git.eclipse.org/r/#/c/37221/
Comment 2 Marc-André Laperle CLA 2015-04-19 13:23:29 EDT
Ran into this again today but this time by pressing the "Reload" button which edits the target file to bump the sequence number. It's not the most urgent fix but perhaps for SR1? :)
Comment 3 Eclipse Genie CLA 2018-03-14 12:54:52 EDT
New Gerrit change created: https://git.eclipse.org/r/119426
Comment 6 Andrey Loskutov CLA 2018-07-14 15:05:28 EDT
(In reply to Eclipse Genie from comment #4)
> Gerrit change https://git.eclipse.org/r/119426 was merged to [master].
> Commit:
> http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/
> ?id=2d42bf841e39599cd26198f6f486191caf22d848

This caused regression on Windows immediately (see http://download.eclipse.org/eclipse/downloads/drops4/I20180625-1545/testresults/html/org.eclipse.pde.ui.tests_ep49I-unit-win32_win32.win32.x86_8.0.html) and on Linux since we have milliseconds in file timestamps. See bug 537037.

BTW, it looks like LocalTargetDefinitionTests aren't executed on Gerrit?
Vikas, where is configured which tests run on Gerrit?
Comment 7 Andrey Loskutov CLA 2018-07-14 15:34:39 EDT
(In reply to Eclipse Genie from comment #4)
> Gerrit change https://git.eclipse.org/r/119426 was merged to [master].
> Commit:
> http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/
> ?id=2d42bf841e39599cd26198f6f486191caf22d848

Also this caused 5 new failures in pde.genericeditor.extension.tests on Windows, because the "golden samples" are saved with Linux line delimiters, and test compares Linux files with output on Windows. See for example http://download.eclipse.org/eclipse/downloads/drops4/I20180713-2000/testresults/html/org.eclipse.pde.genericeditor.extension.tests_ep49I-unit-win32_win32.win32.x86_8.0.html

testMultipleContainersWithSameRepoPersist
org.junit.ComparisonFailure: expected:<...sion="3.8"?><target>[
<locations>
<location includeAllPlatforms="true" includeConfigurePhase="false" includeMode="slicer" includeSource="false" type="InstallableUnit">
<unit id="unit1" version="1.0.0"/>
<unit id="unit2" version="2.0.0"/>
<repository location="TESTURI"/>
</location>
<location includeAllPlatforms="true" includeConfigurePhase="false" includeMode="slicer" includeSource="false" type="InstallableUnit">
<repository location="TESTURI"/>
</location>
</locations>]
</target>> but was:<...sion="3.8"?><target>[
<locations>
<location includeAllPlatforms="true" includeConfigurePhase="false" includeMode="slicer" includeSource="false" type="InstallableUnit">
<unit id="unit1" version="1.0.0"/>
<unit id="unit2" version="2.0.0"/>
<repository location="TESTURI"/>
</location>
<location includeAllPlatforms="true" includeConfigurePhase="false" includeMode="slicer" includeSource="false" type="InstallableUnit">
<repository location="TESTURI"/>
</location>
</locations>
]
</target>>
at org.eclipse.pde.genericeditor.extension.tests.Bug531602FormattingTests.confirmMatch(Bug531602FormattingTests.java:129)
at org.eclipse.pde.genericeditor.extension.tests.Bug531602FormattingTests.testMultipleContainersWithSameRepoPersist(Bug531602FormattingTests.java:99)

@Lucas, please fix those tests.
Comment 8 Vikas Chandra CLA 2018-07-16 02:16:45 EDT
>>Vikas, where is configured which tests run on Gerrit?

Only AllPDEMinimalTests runs on gerrit. Also API tools tests dont run.

So around 437 out of 530 tests run on gerrit.
Comment 9 Andrey Loskutov CLA 2018-07-16 02:43:56 EDT
(In reply to Vikas Chandra from comment #8)
> >>Vikas, where is configured which tests run on Gerrit?
> 
> Only AllPDEMinimalTests runs on gerrit. Also API tools tests dont run.
> 
> So around 437 out of 530 tests run on gerrit.

I see, but *where* is specified, which tests should run on Gerrit / main build and which not? Can we enable missing tests, or they are known to fail on Gerrit?
Comment 10 Vikas Chandra CLA 2018-07-16 05:36:31 EDT
See bug 488036

>> Can we enable missing tests, or they are known to fail on Gerrit?

You can give it another try to enable all tests.
Comment 11 Eclipse Genie CLA 2018-07-16 09:47:14 EDT
New Gerrit change created: https://git.eclipse.org/r/126112
Comment 13 Andrey Loskutov CLA 2018-07-17 08:12:15 EDT
(In reply to Vikas Chandra from comment #10)
> See bug 488036
> 
> >> Can we enable missing tests, or they are known to fail on Gerrit?
> 
> You can give it another try to enable all tests.

Thanks Vikas, I've created bug 537108 to follow up here.
Comment 14 Eclipse Genie CLA 2018-07-18 17:47:17 EDT
New Gerrit change created: https://git.eclipse.org/r/126295
Comment 16 Andrey Loskutov CLA 2018-07-19 05:33:46 EDT
(In reply to Eclipse Genie from comment #15)
> Gerrit change https://git.eclipse.org/r/126295 was merged to [master].
> Commit:
> http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/
> ?id=da0e669267c6805a8caecbed8dec7ed093f264e5

THis shows finally no fails in pde.genericeditor.extension.tests on Windows, see 
http://download.eclipse.org/eclipse/downloads/drops4/I20180718-2000/testResults.php
Comment 17 Andrey Loskutov CLA 2018-12-03 08:05:49 EST
I'm coming from a regression caused by this patch, see bug 541678, and wonder what is the state of this bug? It is still shown as open?
Comment 18 Vikas Chandra CLA 2018-12-04 00:45:38 EST
Marking this as resolved.
Comment 19 Vikas Chandra CLA 2019-02-07 23:56:07 EST
See https://bugs.eclipse.org/bugs/show_bug.cgi?id=541678#c35
Comment 20 Dani Megert CLA 2019-02-08 11:47:06 EST
(In reply to Vikas Chandra from comment #19)
> See https://bugs.eclipse.org/bugs/show_bug.cgi?id=541678#c35
So, you reverted the changes for this bug here? If so, why is the revert not attached here?
Comment 21 Vikas Chandra CLA 2019-02-11 05:49:05 EST
(In reply to Dani Megert from comment #20)
> (In reply to Vikas Chandra from comment #19)
> > See https://bugs.eclipse.org/bugs/show_bug.cgi?id=541678#c35
> So, you reverted the changes for this bug here? If so, why is the revert not
> attached here?

The revert had conflicts. So I had to manually revert it and it didn't appear on this bug.  I have now added it to "See Also".
Comment 22 Dani Megert CLA 2019-02-11 06:05:30 EST
(In reply to Vikas Chandra from comment #21)
> (In reply to Dani Megert from comment #20)
> > (In reply to Vikas Chandra from comment #19)
> > > See https://bugs.eclipse.org/bugs/show_bug.cgi?id=541678#c35
> > So, you reverted the changes for this bug here? If so, why is the revert not
> > attached here?
> 
> The revert had conflicts. So I had to manually revert it and it didn't
> appear on this bug.  I have now added it to "See Also".
Thanks.
Comment 23 Eclipse Genie CLA 2019-02-14 04:31:42 EST
New Gerrit change created: https://git.eclipse.org/r/136899
Comment 24 Vikas Chandra CLA 2019-02-14 04:57:22 EST
Comment#23 has a patch ( WIP) which modifies patch in comment#1

1) For getting line separator from an IFile, TextUtilities.getDefaultLineDelimiter is used

2) For files external to workspace, line separator from InputStream is taken
( ExternalFileTargetHandle)

3) Utility function used at multiple places are now put in CoreUtility

This fixes target editor issues in windows and Mac and doesn't create a new problem while creating a new target file.
Comment 25 Eclipse Genie CLA 2019-02-14 05:12:20 EST
New Gerrit change created: https://git.eclipse.org/r/136903
Comment 26 Dani Megert CLA 2019-02-14 11:30:30 EST
(In reply to Vikas Chandra from comment #24)
> Comment#23 has a patch ( WIP) which modifies patch in comment#1
> 
> 1) For getting line separator from an IFile,
> TextUtilities.getDefaultLineDelimiter is used
> 
> 2) For files external to workspace, line separator from InputStream is taken
> ( ExternalFileTargetHandle)
> 
> 3) Utility function used at multiple places are now put in CoreUtility
> 
> This fixes target editor issues in windows and Mac and doesn't create a new
> problem while creating a new target file.
This is still a hack with lots of (existing) duplicated code. PDE should change all the code to directly use file buffers (see also bug 515662).
Comment 27 Alexander Fedorov CLA 2019-05-16 05:55:54 EDT
The current implementation utilize the javax.xml.transform.Transformer and very sensitive to JRE version. In particular, the line endings may be set (or preserved, who knows) by transformer implementation.

Let's try to use standard PDE way of writing manifests. It is a bit vintage and has a lot of hand-made xml processing, but provides fine control over the file content.

I suggest to move this one to 4.13

@Vikas, @Julian WDYT?
Comment 28 Vikas Chandra CLA 2019-08-12 07:47:51 EDT
(In reply to Alexander Fedorov from comment #27)
> The current implementation utilize the javax.xml.transform.Transformer and
> very sensitive to JRE version. In particular, the line endings may be set
> (or preserved, who knows) by transformer implementation.
> 
> Let's try to use standard PDE way of writing manifests. It is a bit vintage
> and has a lot of hand-made xml processing, but provides fine control over
> the file content.
> 
> I suggest to move this one to 4.13
> 
> @Vikas, @Julian WDYT?

Lets take this up in 4.14 !
Comment 29 Eclipse Genie CLA 2021-08-02 10:27:56 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 30 Eclipse Genie CLA 2021-08-02 12:16:19 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 31 Eclipse Genie CLA 2023-07-24 02:03:45 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.