Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 340660 - More than one options found for kind 1
Summary: More than one options found for kind 1
Status: NEW
Alias: None
Product: CDT
Classification: Tools
Component: cdt-build-managed (show other bugs)
Version: 8.0   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact: Jonah Graham CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-22 10:10 EDT by Andrew Gvozdev CLA
Modified: 2020-09-04 15:22 EDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Gvozdev CLA 2011-03-22 10:10:01 EDT
Once in a while we are getting an exception from managed build plugin:

java.lang.Exception
at org.eclipse.cdt.managedbuilder.core.ManagedBuilderCorePlugin.error(ManagedBuilderCorePlugin.java:226)
at org.eclipse.cdt.managedbuilder.internal.dataprovider.BuildEntryStorage.setUserEntries(BuildEntryStorage.java:610)
at org.eclipse.cdt.managedbuilder.internal.dataprovider.BuildEntryStorage.obtainEntriesFromLevel(BuildEntryStorage.java:152)
at org.eclipse.cdt.core.settings.model.util.AbstractEntryStorage.setEntries(AbstractEntryStorage.java:63)
at org.eclipse.cdt.managedbuilder.internal.dataprovider.BuildLanguageData.setEntries(BuildLanguageData.java:103)
at org.eclipse.cdt.core.settings.model.util.PathEntryTranslator.applyEntries(PathEntryTranslator.java:1496)
at org.eclipse.cdt.core.settings.model.util.PathEntryTranslator.applyEntries(PathEntryTranslator.java:1477)
at org.eclipse.cdt.core.settings.model.util.PathEntryTranslator.applyLangSettings(PathEntryTranslator.java:1408)
at org.eclipse.cdt.core.settings.model.util.PathEntryTranslator.addPathEntries(PathEntryTranslator.java:1234)
at org.eclipse.cdt.core.settings.model.util.PathEntryTranslator.applyPathEntries(PathEntryTranslator.java:1541)
at org.eclipse.cdt.internal.core.settings.model.ConfigBasedPathEntryStore.setRawPathEntries(ConfigBasedPathEntryStore.java:141)
at org.eclipse.cdt.internal.core.model.PathEntryStoreProxy.setRawPathEntries(PathEntryStoreProxy.java:105)
at org.eclipse.cdt.internal.core.model.PathEntryManager.saveRawPathEntries(PathEntryManager.java:1009)
at org.eclipse.cdt.internal.core.model.SetPathEntriesOperation.executeOperation(SetPathEntriesOperation.java:55)
at org.eclipse.cdt.internal.core.model.CModelOperation.execute(CModelOperation.java:339)
at org.eclipse.cdt.internal.core.model.CModelOperation.run(CModelOperation.java:602)
at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2310)
at org.eclipse.cdt.internal.core.model.CModelOperation.runOperation(CModelOperation.java:633)
at org.eclipse.cdt.internal.core.model.PathEntryManager.setRawPathEntries(PathEntryManager.java:636)
at org.eclipse.cdt.internal.core.model.PathEntryManager$2.runInWorkspace(PathEntryManager.java:1458)
at org.eclipse.core.internal.resources.InternalWorkspaceJob.run(InternalWorkspaceJob.java:38)
at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)

Not sure if it makes a difference but we are using customized toolchain defined in our own plugin.
Comment 1 Andrew Gvozdev CLA 2011-03-22 10:13:38 EDT
and the exact message text of the exception is:
"Unexpected error: Warning more than one options found for kind 1"
Comment 2 James Blackburn CLA 2011-03-22 11:39:32 EDT
I added that error... It's an internal error for toolchain integrators though if you've hit it it should be more informative.  

It looks like you have a tool in your toolchain that has two IOptions which have a type of IOption.INCLUDE_PATH (which would be contributed back to cdt.core for ICSettingEntry.INCLUDE_PATH).

Previously the code silently handled this case and only used the settings on the first arbitrarily discovered option.
Comment 3 Andrew Gvozdev CLA 2011-04-08 18:18:35 EDT
You are right, our include entries are nicely organized in options on configuration level and tools combine them via option references. I did not encounter problems with that setup except for the rare entry in the log.

I was able to reproduce the exception adding another include path in "Paths and Symbols". Normally we won't even do that with that toolchain as all the necessary include paths are provided by the toolchain.

What would happen here (in BuildEntryStorage.setUserEntries()) if an entry is added? Do I understand correctly that all the entries will be lumped together and assigned to the first option? If so is this even a problem? I won't care how the entries are organized as long as the final list is correct.
Comment 4 James Blackburn CLA 2011-04-11 04:23:41 EDT
(In reply to comment #3)

I think this is the interface between cdt.core and the MBS.  It's where language setting entries are mapped to IOptions. So if you have multiple IOptions for one LSE, cdt.core probably won't be able to see / alter the settings from one of the IOptions.

If you add include paths under both your IOptions, does Paths & Symbols show the unified set?  I suspect it won't, and also the indexer and the rest of cdt.core won't see them.
Comment 5 Andrew Gvozdev CLA 2011-04-11 10:35:11 EDT
(In reply to comment #4)
> (In reply to comment #3)
> I think this is the interface between cdt.core and the MBS.  It's where language
> setting entries are mapped to IOptions. So if you have multiple IOptions for one
> LSE, cdt.core probably won't be able to see / alter the settings from one of the
> IOptions.
> 
> If you add include paths under both your IOptions, does Paths & Symbols show the
> unified set?  I suspect it won't, and also the indexer and the rest of cdt.core
> won't see them.
Yes it shows the unified set and the indexer uses it just fine. That's my point if I was not clear in the last comment. After an entry added/deleted/edited in UI the full set comes to setUserEntries in "entries" and gets assigned to the first option. Moreover, if you look at the bottom of the function, the code takes trouble to remove all the entries from the other options of that particular kind.

So it seems that this code works correctly even if funky. I suggest to remove the log and instead put a comment to avoid confusion in future.
Comment 6 James Blackburn CLA 2011-04-11 10:45:38 EDT
(In reply to comment #5)
> So it seems that this code works correctly even if funky. I suggest to remove
> the log and instead put a comment to avoid confusion in future.

I don't see how it works correctly.

From what you're saying and looking at the code:
#getUserEntries(): maps: contents(IOption[0-N]) ->  LSE[]
#setUserEntries(): maps: LSE[] -> IOption[0]

Therefore it chooses an _arbitrary_ IOption (the 0'th one) to store the settings on.  Why is the first such IOption better than the rest?

In the case where there is more than one IOption, calling #get followed by #set without changing anything will lead to different stored options, won't it?  In my view this would indicate a bug - hence the warning that the toolchain is funky.
Comment 7 James Blackburn CLA 2011-04-11 10:54:36 EDT
(In reply to comment #6)

More importantly this means that colliding IOptions after the 0th are cleared by the cdt.core interface. This means they're effectively unused: cdt.core won't save settings won't there and existing settings on these IOptions will silently disappear.
Comment 8 Andrew Gvozdev CLA 2011-04-11 11:02:49 EDT
(In reply to comment #6)
> (In reply to comment #5)
> > So it seems that this code works correctly even if funky. I suggest to remove
> > the log and instead put a comment to avoid confusion in future.
> 
> I don't see how it works correctly.
> 
> From what you're saying and looking at the code:
> #getUserEntries(): maps: contents(IOption[0-N]) ->  LSE[]
> #setUserEntries(): maps: LSE[] -> IOption[0]
> Therefore it chooses an _arbitrary_ IOption (the 0'th one) to store the settings
> on.  Why is the first such IOption better than the rest?

The end result for the indexer is the same, isn't it? Why do you care if it comes from different IOptions or from one arbitrary one? 

> In the case where there is more than one IOption, calling #get followed by #set
> without changing anything will lead to different stored options, won't it?  In
> my view this would indicate a bug - hence the warning that the toolchain is
> funky.
Obviously it works for indexing and for all other purposes I can think of. We've been working with such toolchain for years. The schema allows defining multiple options so I do not agree that such toolchain settings are illegal.

Really, can you point to any specific problem if there is any? If you know any we should file a bug against that. Having entries rearranged to different options is not a problem but implementation detail.
Comment 9 Andrew Gvozdev CLA 2011-04-11 11:05:01 EDT
(In reply to comment #7)
> (In reply to comment #6)
> More importantly this means that colliding IOptions after the 0th are cleared by
> the cdt.core interface. This means they're effectively unused: cdt.core won't
> save settings won't there and existing settings on these IOptions will silently
> disappear.
I don't understand how you intend to use colliding options in other way than use one of them and ignore the other. The options are resolved to one list anyway for the indexer and any other client.
Comment 10 James Blackburn CLA 2011-04-11 11:18:49 EDT
(In reply to comment #8)
> Really, can you point to any specific problem if there is any? If you know any
> we should file a bug against that. Having entries rearranged to different
> options is not a problem but implementation detail.

The point is that the options after option[0] are redundant. And there is *data loss* as the precise option on which the settings were set are lost.  So if you: 
 - have two colliding options
 - go into the Build Settings UI and apply settings to both options
 - then change those settings in C/C++ General
the settings will all be moved to one of the two options!

option[0] is a completely _arbitrary_ choice. So the settings on option[1] otpion[2] are corrupted when the user visits an arbitrary property page...  

This will result in:
  - arbitrary changes to the .cproject
  - inconsistency setting options in the UI: options entered through the build settings UI will randomly disappear and re-appear on another option
  - IManagedOptionValueHandler may be affected
  - IOptionApplicability handler
  - Makefile generator
    ...

fundamentally _anything_ that introspects the build model and examines options will expect values set on them to persist.  They may also expect to control enablement, visibility, or the expression of the options' values in makefiles.

The bug is simple, and it's this:  When there's more than one option of a given cdt.core type, the options' values are randomly changed / moved.  This is corruption of the build model. Hence why the warning is there.  It may work for your particular case, but it will not work in general as the code stands.
Comment 11 James Blackburn CLA 2011-04-11 11:21:34 EDT
(In reply to comment #9)
> I don't understand how you intend to use colliding options in other way than
> use one of them and ignore the other. The options are resolved to one list
> anyway for the indexer and any other client.

Then why isn't there only ONE MBS option? 

There're N options in the MBS mapping to ONE option in cdt.core.  The mapping created is _not_ a bijection so data is lost when transferring to cdt.core and back again.

If a tool/toolchain has multiple such colliding options, MBS will not handle them correctly and settings the user applies will randomly change.
Comment 12 Andrew Gvozdev CLA 2011-04-11 12:06:41 EDT
(In reply to comment #10)
> (In reply to comment #8)
> This will result in:
> - arbitrary changes to the .cproject
> - inconsistency setting options in the UI: options entered through the build
> settings UI will randomly disappear and re-appear on another option
> - IManagedOptionValueHandler may be affected
> - IOptionApplicability handler
> - Makefile generator
I see. Thanks for the explanation. Let me update the wording on the exception.
Also, getting the warning at that point is way too late, it really should pop up when the toolchain is created.

This is not real problem for us though as the options parameters are identical. The only purpose of having several of them is to organize, i.e. avoid duplication and be able to apply changes to a logical group of entries to multiple tools of multiple configurations easier.
Comment 13 James Blackburn CLA 2011-04-11 12:14:37 EDT
(In reply to comment #12)
> I see. Thanks for the explanation. Let me update the wording on the exception.
> Also, getting the warning at that point is way too late, it really should pop
> up when the toolchain is created.

Agreed.  We don't do any static checking of the model and this really is just a fallback.  Ideally a check for this should happen much earlier...

> This is not real problem for us though as the options parameters are identical.
> The only purpose of having several of them is to organize, i.e. avoid
> duplication and be able to apply changes to a logical group of entries to
> multiple tools of multiple configurations easier.

By all means make the error configurable.  It would be a couple of lines to add a hidden preference to control build model warnings and you could then have in your product a default something like:
org.eclipse.cdt.managedbuilder.core/warn_toolchain_model_issues=false

As the toolchain which ships with CDT doesn't trip this, it would be good to report it by default.