Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 340181 - Formatter from the command line breaks
Summary: Formatter from the command line breaks
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: Macintosh Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 3.7 RC2   Edit
Assignee: Satyam Kandula CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 344217 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-03-16 12:23 EDT by panayotis CLA
Modified: 2011-05-23 02:41 EDT (History)
6 users (show)

See Also:
srikanth_sankaran: review+
satyam.kandula: review+


Attachments
preferences file (21.40 KB, application/octet-stream)
2011-03-16 12:24 EDT, panayotis CLA
no flags Details
Test case (2.22 KB, application/x-tar)
2011-03-16 12:38 EDT, panayotis CLA
no flags Details
Proposed patch (3.22 KB, patch)
2011-05-17 10:36 EDT, Olivier Thomann CLA
no flags Details | Diff
Proposed fix (3.84 KB, patch)
2011-05-17 14:16 EDT, Olivier Thomann CLA
no flags Details | Diff
Proposed fix (4.95 KB, patch)
2011-05-17 14:18 EDT, Olivier Thomann CLA
no flags Details | Diff
Another patch (3.84 KB, patch)
2011-05-18 05:29 EDT, Satyam Kandula CLA
no flags Details | Diff
Another patch (6.79 KB, patch)
2011-05-18 12:00 EDT, Satyam Kandula CLA
no flags Details | Diff
Patch (6.80 KB, patch)
2011-05-18 12:19 EDT, Satyam Kandula CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description panayotis CLA 2011-03-16 12:23:13 EDT
Build Identifier: 20100218-1602

I am trying to run the formatter from the command line with a command similar to this:
eclipse -nosplash -application org.eclipse.jdt.core.JavaCodeFormatter -verbose -config org.eclipse.jdt.core.prefs src

Unfortunately I get this error message:

A problem occurred while reading the config file .settings/org.eclipse.jdt.core.prefs.

Usage: eclipse -application org.eclipse.jdt.core.JavaCodeFormatter [ OPTIONS ] -config <configFile> <files>

  <files>   Java source files and/or directories to format.
            Only files ending with .java will be formatted in the given directory.
  -config <configFile> Use the formatting style from the specified properties file.
                       Refer to the help documentation to find out how to generate this file.

OPTIONS:

  -help                Display this message.
  -quiet               Only print error messages.
  -verbose             Be verbose about the formatting job.

Reproducible: Always

Steps to Reproduce:
1.create a demo java class under directory src
2. copy org.eclipse.jdt.core.prefs to the current directory
3. run the above mentioned command
Comment 1 panayotis CLA 2011-03-16 12:24:17 EDT
Created attachment 191312 [details]
preferences file
Comment 2 Olivier Thomann CLA 2011-03-16 12:25:30 EDT
Does it work when the same application is used inside the IDE from a lauching configuration ?
Comment 3 panayotis CLA 2011-03-16 12:28:51 EDT
Yes it works fine!
Only when run from the command line it breaks.
Comment 4 panayotis CLA 2011-03-16 12:30:01 EDT
PS in the output it says:
"A problem occurred while reading the config file .settings/org.eclipse.jdt.core.prefs."

The correct is 
"A problem occurred while reading the config file org.eclipse.jdt.core.prefs."

This is from an earlier test which I directly used the file inside the ".settings" folder, while the test command line I gave is when I moved this file to the current directory.
In both cases the situation is the same though.
Comment 5 Olivier Thomann CLA 2011-03-16 12:33:57 EDT
From what directory are you running the command line ?
The parent folder where the src folder is located ?

I'll try to reproduce.
Comment 6 panayotis CLA 2011-03-16 12:38:00 EDT
Created attachment 191313 [details]
Test case

It should not be a path issue.
I also attach my minimum test case.
To use it, 
cd lala
eclipse -nosplash -application org.eclipse.jdt.core.JavaCodeFormatter -verbose -config org.eclipse.jdt.core.prefs src/lala.java
Comment 7 Olivier Thomann CLA 2011-03-16 12:52:27 EDT
It works fine for me using 3.7M6:
...\eclipsec -nosplash -application org.eclipse.jdt.core.JavaCodeFormatter -verbose -config org.eclipse.jdt.core.prefs src\lala.java

osgi> Configuration Name: org.eclipse.jdt.core.prefs
Starting format job ...
Formatting: c:\temp\lala\src\lala.java
Done.

I also used eclipse.exe and it works fine as well.
Could you please try with 3.7M6?

I also tried with 3.6.2 and it worked.

What JRE are you using to run the command line ?
Comment 8 panayotis CLA 2011-03-16 13:21:31 EDT
$ java -version
java version "1.6.0_24"
Java(TM) SE Runtime Environment (build 1.6.0_24-b07-334-10M3326)
Java HotSpot(TM) 64-Bit Server VM (build 19.1-b02-334, mixed mode)

OS version: Mac OS X 10.6.6

I just downloaded 3.7M6: no luck either.


I just tried it also in a stable-debian machine with this version:
$ java -version
java version "1.6.0_11"
Java(TM) SE Runtime Environment (build 1.6.0_11-b03)
Java HotSpot(TM) Server VM (build 11.0-b16, mixed mode)

Again the same problem.
Comment 9 Olivier Thomann CLA 2011-03-16 13:41:55 EDT
I am running on Windows. I'll have access to a linux box (Redhat 5). So I'll give it a try. I don't have access to a mac.
Comment 10 Olivier Thomann CLA 2011-05-13 13:18:27 EDT
*** Bug 344217 has been marked as a duplicate of this bug. ***
Comment 11 Olivier Thomann CLA 2011-05-17 10:29:02 EDT
It seems to be related to relative paths on MacOS.
It fails when opening the stream on the configuration file.

We might need to add debug statements for RC2.

Satyam, since you have access to a Mac, could you please investigate ?
Comment 12 Olivier Thomann CLA 2011-05-17 10:36:52 EDT
Created attachment 195865 [details]
Proposed patch

Add debug statement in verbose mode to see why the call to read the configuration file doesn't work.
Comment 13 Silenio Quarti CLA 2011-05-17 13:20:58 EDT
Ok, now I know what is going on. On MacOS, the "user,dir" property is set according to the running application.
So on the mac I used, it was set to: ..../Eclipse.app/Contents/MacOS.
It was not set according to the directory from which the application was run. This is different from what is done on Windows or Linux.
So the only way to get it to work using relative path is to use '..' until you reach the file you want to target. I don't plan to change anything for this except that we should provide a more helpful message to the user.
Comment 14 panayotis CLA 2011-05-17 13:29:51 EDT
Strange, the behavior with pure Java (I don't know about Eclipse) is not like this.
Consider the following simple code:
public class props {

	public static void main (String args[]) {
            System.out.println(System.getProperty("user.dir"));
	}
}
 

put under (e.g.) ~/Works/Development/Java/properties/props.java

Now these commands:

$ cd ~/Works/Development/Java/properties
$ javac props.java
$ cd ..
$ java -cp properties props
/Users/teras/Works/Development/Java

So it seems that Java under Mac correctly set user.dir in the current directory.
Should it be a problem with the Eclipse executable?
Comment 15 Olivier Thomann CLA 2011-05-17 13:31:33 EDT
(In reply to comment #14)
> Should it be a problem with the Eclipse executable?
It looks like it is. At least on MacOS. On Linux or Windows the appropriate value is set.
Comment 16 Olivier Thomann CLA 2011-05-17 13:32:03 EDT
Sorry I was logged as Silenio on Silenio's Mac, but I set the comment 13.
I'll take it back and prepare a patch to provide a more helpful message to the user.
Satyam, I'll request your review once the patch is ready. You will also be able to double-check the behavior on Mac once a build that contains it is ready.
Comment 17 Andrew Niefer CLA 2011-05-17 13:42:50 EDT
(In reply to comment #14)
> So it seems that Java under Mac correctly set user.dir in the current
> directory.
> Should it be a problem with the Eclipse executable?

See bug 327961, the eclipse executable on the mac natively sets the current working directory to the folder containing the executable.  This is there historically from early versions of eclipse, I don't know why it was required.

A workaround would be to start eclipse by invoking java directly with a command line like:
java -jar /eclipse/plugins/org.eclipse.equinox.launcher _1.2.0.v20110502.jar -application org.eclipse.jdt.core.JavaCodeFormatter
-verbose -config org.eclipse.jdt.core.prefs src\lala.java
Comment 18 Olivier Thomann CLA 2011-05-17 13:56:49 EDT
I will still provide a better error message so that users don't waste their time trying to find out what is wrong.
Comment 19 Olivier Thomann CLA 2011-05-17 14:16:53 EDT
Created attachment 195901 [details]
Proposed fix

The .log file and the current console would now get the following message:
Error reading configuration file (file path : C:\Downloads\TestProject\.settings\org.eclipse.jdt.core.prefs, current user directory used to read the file: C:\Downloads\TestProject).
This would help the user to realize that the expected file is not read.
I also added a note in the javadoc of the code formatter application to mention the potential problems with relative paths and the eclipse launcher.
Comment 20 Olivier Thomann CLA 2011-05-17 14:18:05 EDT
Created attachment 195902 [details]
Proposed fix

Same patch with updated copyrights.
Comment 21 Olivier Thomann CLA 2011-05-17 14:18:24 EDT
Srikanth, Satyam, please review.
Comment 22 Olivier Thomann CLA 2011-05-17 14:19:49 EDT
I think we should target RC2 as there is no risk involved with this patch and we make sure that users of this application on MacOS won't be stucked trying to run it using relative paths.
Comment 23 Satyam Kandula CLA 2011-05-18 01:53:22 EDT
I think it could be better if the message could suggest to use the full path. 
Similar problem exists for the files to be formatted too. 
I will try to propose another patch.
Comment 24 Satyam Kandula CLA 2011-05-18 05:25:08 EDT
(In reply to comment #23)
> I think it could be better if the message could suggest to use the full path. 
> Similar problem exists for the files to be formatted too. 
> I will try to propose another patch.
In retrospective, it is probably fine to not have a suggestion, because the problem is only on MAC and the suggestion will not be correct many cases. As the message shows the full path that is being resolved, one should easily understand. 

At the same time, it will be good to have the full path for the processed files also. I will the patch for that shortly. Please take a look at it.
Comment 25 Satyam Kandula CLA 2011-05-18 05:29:40 EDT
Created attachment 195941 [details]
Another patch

Patch as mentioned in comment 24.
Comment 26 Satyam Kandula CLA 2011-05-18 05:32:14 EDT
Though I have proposed a new patch to have a better message for processing files, patch proposed in comment 20 is good and safe for RC2. +1.
Comment 27 panayotis CLA 2011-05-18 05:53:46 EDT
I believe, from the user point of view, the best thing would be as follows:

a) At any case display the files that the user provided.

b) If there is an error, then check if this file has absolute pathname (with isAbsolute()), and if it is not display something like "File can not be found etc etc  ... ... It is recommended (especially in Mac OS X) to provide a full pathname of the file".

c) If the file is already absolute, display a regular error message.



Thus, it will promptly inform the user what is wrong, how to fix it and in which operating system to be more careful.
Comment 28 Srikanth Sankaran CLA 2011-05-18 07:10:16 EDT
(In reply to comment #21)
> Srikanth, Satyam, please review.

Patch looks fine. +1.

IMHO, there is sufficient information in the message to unblock a
user.
Comment 29 Olivier Thomann CLA 2011-05-18 08:56:31 EDT
Satyam, please release your last patch.
Comment 30 panayotis CLA 2011-05-18 09:17:53 EDT
(In reply to comment #28)
> 
> IMHO, there is sufficient information in the message to unblock a
> user.

Well, IMHO this message was not obvious for me.
I would still try to understand why the path was just a random path, and not the one I gave.
Comment 31 Satyam Kandula CLA 2011-05-18 12:00:12 EDT
Created attachment 195998 [details]
Another patch

Another patch which checks for absolouteness and suggests to try out full path.
Comment 32 Satyam Kandula CLA 2011-05-18 12:01:43 EDT
(In reply to comment #30)
Please try the patch in comment 31 and let us know if you think that is good.
Comment 33 panayotis CLA 2011-05-18 12:05:38 EDT
Although I don't know the internals of Eclipse,  the error message makes much more sense now.
Thank you!
I think it is fine.
Comment 34 Olivier Thomann CLA 2011-05-18 12:08:38 EDT
+1 for the last patch, but I would use "absolute path" instead of "full path". The problem comes from the relative paths on MacOS. So it is relative vs absolute.
Comment 35 panayotis CLA 2011-05-18 12:09:57 EDT
(In reply to comment #34)
> +1 for the last patch, but I would use "absolute path" instead of "full path".
> The problem comes from the relative paths on MacOS. So it is relative vs
> absolute.

+1
Comment 36 Satyam Kandula CLA 2011-05-18 12:17:11 EDT
(In reply to comment #33)
> Although I don't know the internals of Eclipse,  the error message makes much
> more sense now.
> Thank you!
> I think it is fine.
Thanks, Will update another patch taking your's and Olivier's comments.
Comment 37 Olivier Thomann CLA 2011-05-18 12:17:45 EDT
(In reply to comment #35)
> +1
Thanks for the bug report and your continuous feedbacks.
Comment 38 Satyam Kandula CLA 2011-05-18 12:19:08 EDT
Created attachment 196001 [details]
Patch
Comment 39 Satyam Kandula CLA 2011-05-18 12:36:07 EDT
Released on HEAD
Comment 40 Jay Arthanareeswaran CLA 2011-05-23 02:41:04 EDT
Verified for 3.7 RC2 with build I20110521-0800.