Bug 441654 - build.properties editor should warn on missing bin.includes=.,\
Summary: build.properties editor should warn on missing bin.includes=.,\
Status: VERIFIED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.17   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 4.23 M1   Edit
Assignee: Vikas Chandra CLA
QA Contact:
URL:
Whiteboard:
Keywords: investigate
Depends on:
Blocks: 578093
  Show dependency tree
 
Reported: 2014-08-13 03:21 EDT by Marco Descher CLA
Modified: 2022-01-19 22:46 EST (History)
7 users (show)

See Also:
Vikas.Chandra: review? (curtis.windatt.public)


Attachments
graphical and source visualization (128.80 KB, image/png)
2014-08-13 03:21 EDT, Marco Descher CLA
no flags Details
patch (1.16 KB, patch)
2014-08-14 05:23 EDT, Vikas Chandra CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marco Descher CLA 2014-08-13 03:21:30 EDT
Created attachment 245938 [details]
graphical and source visualization

The entry 

bin.includes = .,\

is not represented in the graphical editor of the build.properties file. If this entry is missing, the jars generated however do miss all the class files. 

The  build.properties editor should definitely show a warning in the case of this entry being missing, such that the user can act accordingly.
Comment 1 Vikas Chandra CLA 2014-08-14 05:23:21 EDT
Created attachment 245988 [details]
patch

Not sure if we should check for "." library in bin.includes but I have seen few people mentioning this problem in forums.


So if we want to put this warning this patch should help.

But it needs some investigation.
Comment 2 Vikas Chandra CLA 2014-09-18 09:41:06 EDT
Curtis, I have seen the same issues mentioned in forums as well. If yes, should we have this error/warning. If that is the case, we should check for "." library.
What do you think?
Comment 3 Curtis Windatt CLA 2014-09-18 15:20:13 EDT
In my experience, and likely the case for new plug-in devs, you almost always use the root of the project as a binary include.  However, there are plug-ins that use library locations and don't need the root included.

Always warning the user is incorrect.  It must be possible to remove the warning.

You can do this with a preference, like the other build prop preferences, it can be set at the project level to handle the specific cases.

Perhaps there is a heuristic we can use to guess whether the user would want the project root included, but I don't have a good suggestion.
Comment 4 Marco Descher CLA 2020-12-03 04:07:58 EST
Just again spent like 3 hours to find a bug which boiled down to this ***** problem!!!

Would definitely like to roll this problem up again!
Comment 5 Marco Descher CLA 2020-12-03 04:10:00 EST
(In reply to Curtis Windatt from comment #3)
> In my experience, and likely the case for new plug-in devs, you almost
> always use the root of the project as a binary include.  However, there are
> plug-ins that use library locations and don't need the root included.
> 
> Always warning the user is incorrect.  It must be possible to remove the
> warning.
> 
> You can do this with a preference, like the other build prop preferences, it
> can be set at the project level to handle the specific cases.
> 
> Perhaps there is a heuristic we can use to guess whether the user would want
> the project root included, but I don't have a good suggestion.

I think, that if within your projects root directory there is a /src folder, and you have classes in there - it should be my target to have them included in the resulting jar file (which is not the case for Tycho if '.' is not mentioned). 

So the warning could be active if there are resp. source classes located.
Comment 6 Alexander Kurtakov CLA 2021-01-07 03:07:51 EST
Mass move 4.19 M1 bugs to M3
Comment 7 Alexander Kurtakov CLA 2021-02-15 04:30:35 EST
Remove target milestone as it seems to not be actively worked on. Please set one when you start working on it.
Comment 8 Eclipse Genie CLA 2021-12-28 05:19:05 EST
New Gerrit change created: https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/189167
Comment 10 Vikas Chandra CLA 2021-12-29 02:24:03 EST
verified on
Version: 2022-03 (4.23)
Build id: I20211228-1800
Comment 11 Hannes Wellmann CLA 2022-01-14 06:47:29 EST
While this bug has bitten me and my colleges multiple times and I appreciate it was addressed, the implemented fix is too strict:
A missing '.' is now also marked (as warning or error) if a plug-in project does not have a default library because it doesn't have any source folders.

Not having a source-folder is for example the case for fragments that only contain platform specific native libraries.
Comment 12 Eclipse Genie CLA 2022-01-14 06:48:28 EST
New Gerrit change created: https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/189633
Comment 13 Eike Stepper CLA 2022-01-17 02:10:30 EST
(In reply to Hannes Wellmann from comment #11)
> [...] the implemented fix is too strict:
> A missing '.' is now also marked (as warning or error) if a plug-in project
> does not have a default library because it doesn't have any source folders.

Yes, I have several projects of that type, too.

And I have an additional, different case! I author several documentation plugins by writing Java source code files. The documentation is then built with the help of a JavaDoc doclet, which produce HTML output. This HTML content is all that I want in my deployable plugin, not the Java classes.

I really can not accept any warning markers in my workspace. I'm aware that I could set the project-level severity for "bin.include" problems to "Ignore", i.e., "compilers.p.build.bin.includes=2", but that would hide other, real problems that I would prefer to see, just like before this nice change.

I think this new "missing ." check justifies, or even requires, a new problem severity setting, as well. What do you think?
Comment 14 Hannes Wellmann CLA 2022-01-17 05:22:54 EST
(In reply to Eike Stepper from comment #13)
> 
> I think this new "missing ." check justifies, or even requires, a new
> problem severity setting, as well. What do you think?

Would it be possible/acceptable for you to place the java-files in a dedicated fragment of the corresponding documentation plug-in or to place them in another plug-in that is referenced by the doc-plugin?
Then you would not have a src-folder in the actual documentation plug-in but the java classes would be available at runtime (depending if the application is a 'pure' Java application or an OSGi application).

If not it looks like a new problem preference-option is required. But since there are already many options in Preferences -> PDE -> Compilers -> Plug-ins -> Build we should add more options only with care.
Comment 16 Vikas Chandra CLA 2022-01-17 05:39:10 EST
>>Yes, I have several projects of that type, too.

These should be fixed now because of Hannes's changes. Use today's I build or later.
Comment 17 Hannes Wellmann CLA 2022-01-17 05:40:34 EST
(In reply to Hannes Wellmann from comment #14)
> (In reply to Eike Stepper from comment #13)
> > 
> > I think this new "missing ." check justifies, or even requires, a new
> > problem severity setting, as well. What do you think?
> 
With the just merged change the warning should also vanish if you remove the 'output..' entry while having "Missing 'output.<library>' entry" set to Ignore.

This 'feature' was actually not intended but it could help in your case, please see my latest comment in the just merged Gerrit.
Comment 18 Vikas Chandra CLA 2022-01-17 05:46:01 EST
Adding a new problem severity "may" be an overkill ( unless there is no other solution)

In the past, for unresolved import, unresolved option dependency always was reported at 1 level lower than in preference ( because it was an optional dependency). If we try the same for this scenario, this would be reported as INFO whereas others bin include problems will be shown as WARNING. I don't really like this option as well but this is an option nevertheless.


Lets check in tomorrow's I build first and see if there is a scenario causing issues. And we would possibly take it from there.
Comment 19 Vikas Chandra CLA 2022-01-17 05:53:20 EST
>please see my latest comment in the just merged Gerrit.

I will have a look
Comment 20 Eike Stepper CLA 2022-01-17 05:54:24 EST
(In reply to Hannes Wellmann from comment #14)

Thanks for the very fast reply!

> Would it be possible/acceptable for you to place the java-files in a
> dedicated fragment of the corresponding documentation plug-in or to place
> them in another plug-in that is referenced by the doc-plugin?
> Then you would not have a src-folder in the actual documentation plug-in but
> the java classes would be available at runtime (depending if the application
> is a 'pure' Java application or an OSGi application).

No, for me it's not acceptable to split existing projects that always used to be error and warning free. I doubt that anyone who didn't ask for this new check (which does make sense in many cases!) would want to deeply change herproject structure just to stay warning-free.

> If not it looks like a new problem preference-option is required. But since
> there are already many options in Preferences -> PDE -> Compilers ->
> Plug-ins -> Build we should add more options only with care.

I think a new option is required in this case because it's a new check that was added, and apparently this new check is not (can not be) adequate in all legal cases.
Comment 21 Hannes Wellmann CLA 2022-01-17 14:17:48 EST
(In reply to Eike Stepper from comment #20)
> (In reply to Hannes Wellmann from comment #14)
> 
> Thanks for the very fast reply!
> 
> > Would it be possible/acceptable for you to place the java-files in a
> > dedicated fragment of the corresponding documentation plug-in or to place
> > them in another plug-in that is referenced by the doc-plugin?
> > Then you would not have a src-folder in the actual documentation plug-in but
> > the java classes would be available at runtime (depending if the application
> > is a 'pure' Java application or an OSGi application).
> 
> No, for me it's not acceptable to split existing projects that always used
> to be error and warning free. I doubt that anyone who didn't ask for this
> new check (which does make sense in many cases!) would want to deeply change
> herproject structure just to stay warning-free.
> 
Understand that.

(In reply to Hannes Wellmann from comment #17)
> With the just merged change the warning should also vanish if you remove the
> 'output..' entry while having "Missing 'output.<library>' entry" set to
> Ignore.
> 
> This 'feature' was actually not intended but it could help in your case,
> please see my latest comment in the just merged Gerrit.

Can you please try if that would work for you?
If yes we could extend the problem message to point to this solution in case someone is in a similar situation?

This 'feature' was not intended, but since the preference to ignore "Missing 'output.<library>' entry"-problems seems to be more precise than the other one about "Problems with 'source.<library>' entry". So I think it is more suitable to perform the check for a '.' in the bin.includes only if a output.. exists and not only if source.. exists, because the first can be removed and ignored with a more precise preference than the latter.
Comment 22 Eike Stepper CLA 2022-01-17 23:57:12 EST
(In reply to Hannes Wellmann from comment #21)

Yes, I confirm that with the I20220117-1800 build

a) the warnings in projects without source folders all disappeared, and
b) removing the "output..=bin/" entry eliminates the remaining warnings.

Good job!

I yet have to verify that our build still produces the correct bits, but I have a good feeling. Thank you very much for taking our concerns so seriously ;-)
Comment 23 Hannes Wellmann CLA 2022-01-19 03:04:45 EST
(In reply to Eike Stepper from comment #22)
> (In reply to Hannes Wellmann from comment #21)
> 
> Yes, I confirm that with the I20220117-1800 build
> 
> a) the warnings in projects without source folders all disappeared, and
> b) removing the "output..=bin/" entry eliminates the remaining warnings.
> 
> Good job!

Thank you and thanks for the verification.

> 
> I yet have to verify that our build still produces the correct bits, but I
> have a good feeling. Thank you very much for taking our concerns so
> seriously ;-)

You're welcome. :)
Since you did not yet notified us about the opposite, I assume your build is still successful? If this is not the case please let us know.
Comment 24 Eike Stepper CLA 2022-01-19 22:46:43 EST
(In reply to Hannes Wellmann from comment #23)
> Since you did not yet notified us about the opposite, I assume your build is
> still successful? If this is not the case please let us know.

Yes, I double-checked and all is good. Thanks again!