Bug 349791

Comment 1 Andrew Gvozdev 2011-06-20 10:52:36 EDT

Petri, could you explain the basic flow how the entries are getting to MBS? In particular, when and where pkg-config command is run and how the options get to CExternalSettingProvider?
My concern with ICSettingEntry.OTHER_FLAG is that ICSettingEntry is part of cdt.core, not of any build packages and I tend to think that using it to carry options that are used only internally in build packages is not right conceptually. ICSettingEntry in cdt.core is used to tie options to the indexer, (and supporting UI i.e. Paths&Symbols and PE) and AFAICS the indexer only. ICSettingEntry.OTHER_FLAGS would not be used in core as the indexer does not need the options you need to pass. Please, clarify the flow so I would understand the proposal better.

Comment 2 Petri Tuononen 2011-06-20 11:45:10 EDT

(In reply to comment #1)
> Petri, could you explain the basic flow how the entries are getting to MBS? In
> particular, when and where pkg-config command is run and how the options get to
> CExternalSettingProvider?
> My concern with ICSettingEntry.OTHER_FLAG is that ICSettingEntry is part of
> cdt.core, not of any build packages and I tend to think that using it to carry
> options that are used only internally in build packages is not right
> conceptually. ICSettingEntry in cdt.core is used to tie options to the indexer,
> (and supporting UI i.e. Paths&Symbols and PE) and AFAICS the indexer only.
> ICSettingEntry.OTHER_FLAGS would not be used in core as the indexer does not
> need the options you need to pass. Please, clarify the flow so I would
> understand the proposal better.

First I create a ProcessBuilder with suitable commands (e.g. pkg-config --cflags pkg_name) and run the process to get similar output one would get from a command line. Secondly I use custom parser which outputs String arrays containing other flags, includes, libs and lib paths (in suitable format for Tool's Options).

I created a property tab under C/C++ Build -> Settings which lists the packages that pkg-config utility finds. Clicking a check box, double clicking or clicking the Select button will trigger handleCheckStateChange() which in turn triggers updateData(getResDesc()) and sets and updates external setting providers (ICConfigurationDescription#setExternalSettingsProviderIds & ICConfigurationDescription#updateExternalSettingsProviders). These two latter methods will trigger a class which extends CExternalSettingProvider and it's method getSettings(). This method needs to pass CExternalSetting[] which contains everything I wish to pass to MBS (Tool's Options). I create separate CExternalSetting for every type (includes etc.) and pass ICSettingEntry[] containing the values.

Basically I run through every package's includes etc. which I get from the custom parser adding them to a list and back to String array. Finally I need to convert this String array to CIncludePathEntry array. Example
CIncludePathEntry incPathEntry = new CIncludePathEntry(new Path(inc), ICSettingEntry.INCLUDE_PATH); creates a correct include path entry.

In short I am struggling to find a way to pass other than just includes and library files and paths to Tool's Options. Currently I don't see a way one could create ICSettingEntry[] containing other flags and passing them to CExternalSetting so that they would be added correctly to MBS.

Sure there is another way to pass other flags to MBS but as far as I know not a dynamic way such as External Setting Provider. It would also be rather convenient to use same approach for other flags.

Comment 3 Andrew Gvozdev 2011-06-20 12:15:56 EDT

(In reply to comment #2)
> First I create a ProcessBuilder with suitable commands (e.g. pkg-config --cflags
> pkg_name) and run the process to get similar output one would get from a command
> line. Secondly I use custom parser which outputs String arrays containing other
> flags, includes, libs and lib paths (in suitable format for Tool's Options).
Where this process is called from? Is it run from your CExternalSettingProvider and triggered by user clicking the check box in UI?

Comment 4 Petri Tuononen 2011-06-20 13:59:50 EDT

(In reply to comment #3)
> (In reply to comment #2)
> > First I create a ProcessBuilder with suitable commands (e.g. pkg-config --cflags
> > pkg_name) and run the process to get similar output one would get from a command
> > line. Secondly I use custom parser which outputs String arrays containing other
> > flags, includes, libs and lib paths (in suitable format for Tool's Options).
> Where this process is called from? Is it run from your CExternalSettingProvider
> and triggered by user clicking the check box in UI?

Exactly.

Comment 5 James Blackburn 2011-06-20 15:44:40 EDT

(In reply to comment #1)
> My concern with ICSettingEntry.OTHER_FLAG is that ICSettingEntry is part of
> cdt.core, not of any build packages and I tend to think that using it to carry
> options that are used only internally in build packages is not right
> conceptually. ICSettingEntry in cdt.core is used to tie options to the indexer,

I'm not sure this is right.  SettingEntries are definitely the core representation of MBS options.  But the point of the External Settings Provider extension point is to provide settings for the build system:

"The external settings provider would be used to specify provider of include/macro/libraryan settings to be used/applied for the build configuration associated with this provider. Any number of setting providers can be associated with the build configurations. This functionality might be used, e.g. by the External SDKs to allow automatic andjustment of the project settings for the projects using thes SDKs, e.g. adding include paths, symbols, etc"

This is distinct from scanner discovery which is just for providing settings for the indexer.

AFAIK the external settings provider is the only way of providing runtime changeable SDK paths to the build system.  Do you know of another way?

Comment 6 Andrew Gvozdev 2011-06-21 15:35:30 EDT

(In reply to comment #5)
> (In reply to comment #1)
> > My concern with ICSettingEntry.OTHER_FLAG is that ICSettingEntry is part of
> > cdt.core, not of any build packages and I tend to think that using it to carry
> > options that are used only internally in build packages is not right
> > conceptually. ICSettingEntry in cdt.core is used to tie options to the
> indexer,
> I'm not sure this is right.  SettingEntries are definitely the core
> representation of MBS options.  But the point of the External Settings Provider
> extension point is to provide settings for the build system:
> "The external settings provider would be used to specify provider of
> include/macro/libraryan settings to be used/applied for the build configuration
> associated with this provider. Any number of setting providers can be associated
> with the build configurations. This functionality might be used, e.g. by the
> External SDKs to allow automatic andjustment of the project settings for the
> projects using thes SDKs, e.g. adding include paths, symbols, etc"
This is the declared purpose of course but to fulfill that the extension point and the SettingEntries do not have to be in core plugin, MBS plugin would be sufficient. The ultimate reason to have the settings in the core is that they are used by the core, and the indexer is the only client using them. I find it telling that the extension point is limited to "include/macro/libraryan settings" not to arbitrary compiler option.
Anyway, I think original intention is good to know but the question is if API change of injecting ICSettingEntry.OTHER_FLAG into core makes it better or worse design-wise.

> This is distinct from scanner discovery which is just for providing settings for
> the indexer.
>
> AFAIK the external settings provider is the only way of providing runtime
> changeable SDK paths to the build system.  Do you know of another way?
Petri described that he needs to change options from properties UI. I would think that changing the options the same way as Tool Settings properties do would be more straightforward, no? If API is internal or missing, won't it be a better idea to add the API in MBS rather than in the core?

Comment 7 James Blackburn 2011-06-21 15:50:07 EDT

Point taken.

(In reply to comment #6)
> Petri described that he needs to change options from properties UI. I would
> think that changing the options the same way as Tool Settings properties do
> would be more straightforward, no? If API is internal or missing, won't it be a
> better idea to add the API in MBS rather than in the core?

It's not an issue of API, rather that the current miscellaneous flags are shared and editable by the user.  It's then impossible to know which entries have been entered by the pkg-config tab and which settings the user typed in manually.

Perhaps a better option would be to add an IOption to the tools called something like 'internal.sdk.flags'.  This could be hidden from the user in the normal settings tab and managed by the pkg-config tab (with perhaps a UI field there to allow the user to override)?

Comment 8 Petri Tuononen 2011-06-25 12:51:28 EDT

(In reply to comment #7)
> Point taken.
> 
> (In reply to comment #6)
> > Petri described that he needs to change options from properties UI. I would
> > think that changing the options the same way as Tool Settings properties do
> > would be more straightforward, no?

The current trunk version is implementad as such, but External Setting Provider approach is more suitable in this case. I have already implemented the latter version, but it is not production quality without other flags.

> It's not an issue of API, rather that the current miscellaneous flags are
> shared and editable by the user.  It's then impossible to know which entries
> have been entered by the pkg-config tab and which settings the user typed in
> manually.

Good explanation.

> 
> Perhaps a better option would be to add an IOption to the tools called
> something like 'internal.sdk.flags'.  This could be hidden from the user in the
> normal settings tab and managed by the pkg-config tab (with perhaps a UI field
> there to allow the user to override)?

This could be done pretty easily I guess but I would avoid these kind of hacks. What if the user wants to use a custom toolchain? Anyway this means that the plug-in won't be supported by older CDT versions which is a minor nuisance, because it seems that the end-users don't update as often as developers might like to think.

Whatever the solution might be it is essential that we can implement it as soon as possible in order to progress further with pkg-config support.

Comment 9 Petri Tuononen 2011-06-25 13:02:51 EDT

(In reply to comment #7)

> Perhaps a better option would be to add an IOption to the tools called
> something like 'internal.sdk.flags'.  This could be hidden from the user in the
> normal settings tab and managed by the pkg-config tab (with perhaps a UI field
> there to allow the user to override)?

This would certainly separate shared miscellaneous flags from pkk-config specific flags. However the dynamic approach of External Settings Provider is lost. This approach does not introduce too much overhead to check OS and override the existing e.g. 'internal.sdk.flags' therefore it would certainly work.

Comment 10 Andrew Gvozdev 2011-06-25 16:35:55 EDT

(In reply to comment #8)
> (In reply to comment #7)
> > Point taken.
> > (In reply to comment #6)
> > > Petri described that he needs to change options from properties UI. I would
> > > think that changing the options the same way as Tool Settings properties do
> > > would be more straightforward, no?
> The current trunk version is implementad as such, but External Setting Provider
> approach is more suitable in this case. I have already implemented the latter
> version, but it is not production quality without other flags.
I do not understand why it is more suitable. If the problem with user flags is resolved, would it be suitable?

> > It's not an issue of API, rather that the current miscellaneous flags are
> > shared and editable by the user.  It's then impossible to know which entries
> > have been entered by the pkg-config tab and which settings the user typed in
> > manually.
> Good explanation.
Could you clarify how External Setting Provider resolves this problem? How is the conflict between this extension point settings and user entries in Tool Settings is resolved in MBS currently?
There is applicabilityCalculator in option schema, it can be used to hide an option from user or to gray out. Can't this mechanism be used to solve this problem?

> > Perhaps a better option would be to add an IOption to the tools called
> > something like 'internal.sdk.flags'.  This could be hidden from the user in
> the
> > normal settings tab and managed by the pkg-config tab (with perhaps a UI field
> > there to allow the user to override)?
> This could be done pretty easily I guess but I would avoid these kind of hacks.
> What if the user wants to use a custom toolchain? Anyway this means that the
> plug-in won't be supported by older CDT versions which is a minor nuisance,
> because it seems that the end-users don't update as often as developers might
> like to think.
If you change ICSettingEntry API and add a new class in cdt.core your plugin can't be used by older CDT versions neither.
I think that if we are lacking a way of providing runtime changeable options to the build system the right way is to add an extension point in MBS to do just that, or maybe amend buildDefinitions extension point if more appropriate.

> Whatever the solution might be it is essential that we can implement it as soon
> as possible in order to progress further with pkg-config support.
Sorry for banality but it would be better to do it right rather than fast. Give us some arguments how your solution is better instead :)

Comment 11 Petri Tuononen 2011-06-26 06:32:49 EDT

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Point taken.
> > > (In reply to comment #6)
> > > > Petri described that he needs to change options from properties UI. I would
> > > > think that changing the options the same way as Tool Settings properties do
> > > > would be more straightforward, no?
> > The current trunk version is implemented as such, but External Setting Provider
> > approach is more suitable in this case. I have already implemented the latter
> > version, but it is not production quality without other flags.
> I do not understand why it is more suitable. If the problem with user flags is
> resolved, would it be suitable?
> 

Like James mentioned, it is because External Setting Provider is dynamic. To provide cross-platform support for pkg-config plug-in it is necessary that the Tool's Options are not hard-coded. File-system hierarchy is different so the paths need to be recalculated again. This happens every time the External Setting Provider is triggered.
If these paths would be hard-coded it would be pretty difficult to determine which paths to remove before adding new paths.

> > > It's not an issue of API, rather that the current miscellaneous flags are
> > > shared and editable by the user.  It's then impossible to know which entries
> > > have been entered by the pkg-config tab and which settings the user typed in
> > > manually.
> > Good explanation.
> Could you clarify how External Setting Provider resolves this problem? How is
> the conflict between this extension point settings and user entries in Tool
> Settings is resolved in MBS currently?
> There is applicabilityCalculator in option schema, it can be used to hide an
> option from user or to gray out. Can't this mechanism be used to solve this
> problem?
> 

Because the External Setting Provider knows which paths belong to it and removes the old values when the new values are added. This resolves the cross-platform issue. This approach is also pretty neat and much easier and faster to implement in the first place.

> > > Perhaps a better option would be to add an IOption to the tools called
> > > something like 'internal.sdk.flags'.  This could be hidden from the user in
> > the
> > > normal settings tab and managed by the pkg-config tab (with perhaps a UI field
> > > there to allow the user to override)?
> > This could be done pretty easily I guess but I would avoid these kind of hacks.
> > What if the user wants to use a custom toolchain? Anyway this means that the
> > plug-in won't be supported by older CDT versions which is a minor nuisance,
> > because it seems that the end-users don't update as often as developers might
> > like to think.
> If you change ICSettingEntry API and add a new class in cdt.core your plugin
> can't be used by older CDT versions neither.
> I think that if we are lacking a way of providing runtime changeable options to
> the build system the right way is to add an extension point in MBS to do just
> that, or maybe amend buildDefinitions extension point if more appropriate.
> 

You are right here that fixing this bug eventually means that it can't be used by older CDT versions. We just have to live with that.
I have to add that I am not sure what the perfect solution would be, but a solution that would be good in a long run (to prevent similar issues).

> > Whatever the solution might be it is essential that we can implement it as soon
> > as possible in order to progress further with pkg-config support.
> Sorry for banality but it would be better to do it right rather than fast. Give
> us some arguments how your solution is better instead :)

Well the reason I said fast is that I can't really progress further on my Google Summer of Code project (pkg-config support) without this 'other flag' problem being solved. Initially it was actually Doug who mentioned it would be better to use External Setting Provider which job is to keep track of entries that belong to the provider and provide the entries dynamically.
Like I said I implemented a version which kind of works except not cross-platform. The another reason which makes External Setting Provider superior is the fact that pkg-config specific entries are stored by the provider which makes it easy to override them.

Comment 12 Petri Tuononen 2011-08-15 08:04:43 EDT

I have been thinking a solution for this problem for a (long) while.

If it's not a good idea to extend external setting provider the only solution that comes to my mind right now is to add IOption 'pkg-config.other.flags' what James mentioned in Comment 7. That field would only be used by pkg-config plug-in. However it's not totally clear for me how it works. Imagine I would show 'other flags' field in pkg-config property tab and when checking packages the field gets populated. However this IOption would not work because it is not part of build definitions. Could this IOption work as a storage and the values be appended to miscellaneous flag IOption of GCC? Then there is also this old dilemma at which point should we check if the OS is different than the flags belonging to the 'other flags' field (and should be recalculated)?

This solution sounds more like hack than a fix for me so feel free to suggest better ideas that are possible to implement (without major changes to APIs etc).