Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

-Dosgi.dataAreaRequiresExplicitInit=true doesn't work anymore #35

Closed
iloveeclipse opened this issue Apr 22, 2022 · 7 comments
Closed

-Dosgi.dataAreaRequiresExplicitInit=true doesn't work anymore #35

iloveeclipse opened this issue Apr 22, 2022 · 7 comments
Assignees
@iloveeclipse
Labels
bug Something isn't working regression

Comments

@iloveeclipse
Copy link
Member

This was originally created as eclipse-equinox/equinox.bundles#21.

While reviewing eclipse-platform/eclipse.platform.resources#71 I've found, that I'm able to silently init a workspace (with default value) without specifying workspace location explicitly via prompt or -data argument, even if -Dosgi.dataAreaRequiresExplicitInit=true is set in eclipse.ini.

This is a regression, so bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=514333 is back.

I'm investigating where it was broken... 4.19 is OK, 4.20 is broken...

OK, regression is coming from

Ironically, commit message was The start code for the plugin can be migrated to run inside Workspace without changing existing behaviour....

The root cause is the default initialization order of LocalMetaArea and WorkspaceRoot objects after the patch above. If the WorkspaceRoot is created first, it silently initializes default location for workspace. I will push a patch.
@iloveeclipse iloveeclipse self-assigned this Apr 22, 2022
@iloveeclipse iloveeclipse added bug Something isn't working regression labels Apr 22, 2022
@laeubi
Copy link
Contributor

laeubi commented Apr 22, 2022

Just wondering what osgi.dataAreaRequiresExplicitInit does? I though there is no default for the instance (aka workspace) location defined at all so what default is then used here? I also can't find osgi.dataAreaRequiresExplicitInit in the patch is is used somewhere else?

@laeubi
Copy link
Contributor

laeubi commented Apr 22, 2022

Okay I see it in DataArea.REQUIRES_EXPLICIT_INIT so can you explain if you see that the exception occurs there? DataArea is another part where we really need some better handling but ResourcePlugin should not trigger anything here anymore and WorkspaceRoot is only created if the instance location is set by the user so I'm a bit confused :-)

@iloveeclipse
Copy link
Member Author

I will explain later. I've found another issue that I would like to debug and understand first.

@laeubi
Copy link
Contributor

laeubi commented Apr 22, 2022

No problem, let me know if I can assist here and thanks for this very detailed review!

@iloveeclipse iloveeclipse mentioned this issue Apr 22, 2022
Closed
@iloveeclipse iloveeclipse transferred this issue from eclipse-platform/eclipse.platform.resources Apr 22, 2022
iloveeclipse added a commit to iloveeclipse/eclipse.platform.runtime that referenced this issue Apr 22, 2022
@iloveeclipse
Added check for not intended default workspace location use (eclipse-…
be3f420 
…platform#35)

See original bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=514333
that was only partly fixed.

The original fix added -Dosgi.dataAreaRequiresExplicitInit=true system
property and changed org.eclipse.core.internal.runtime.DataArea in the
way that it doesn't allow to initialize itself if the workspace location
was not explicitly specified yet - either via -data or via workspace
selection prompt.

But the fix in DataArea does not work if bad client code calls
Platform.getLocation() *before* calling Plugin.getStateLocation(),
because Platform.getLocation() doesn't run into changed DataArea code at
all.

Exact this happened in commit
eclipse-platform/eclipse.platform.resources@a8a8d82
where workspace init order was slightly changed: original code
initialized LocalMetaArea first (which then failed as supposed in
assertLocationInitialized() if workspace was not set yet), and after
that initialized WorkspaceRoot (which uses Platform.getLocation() after
the check). Changed code initialized WorkspaceRoot first, and because
this uses Platform.getLocation(), the code silently initializes default
workspace location even if -Dosgi.dataAreaRequiresExplicitInit=true is
set.

The current fix makes sure that if the system flag is given, we disallow
silent workspace location initialization by adding similar check in
Platform.getLocation().

Fixes issue
eclipse-platform#35
@iloveeclipse iloveeclipse mentioned this issue Apr 22, 2022
Merged
@iloveeclipse
Copy link
Member Author

iloveeclipse commented Apr 22, 2022 •
edited

I though there is no default for the instance (aka workspace) location defined at all so what default is then used here

Unfortunately exactly that is the problem. BasicLocation, if not explicitly set, resolves to default in org.eclipse.osgi.internal.location.BasicLocation.getURL(), and default in Equinox is defined as userhome/workspace, see org.eclipse.osgi.internal.location.EquinoxLocations.EquinoxLocations().

For the rest, see explanation in #36 commit.

Example "bad" bundle that initializes workspace in parallel to the prompt: iloveeclipse/java-things@1dea941

iloveeclipse added a commit to iloveeclipse/java-things that referenced this issue Apr 22, 2022
@iloveeclipse
Bad workspace init bundle example
1dea941 
Bundle uses immediate service to init workspace to default location,
before prompt is even shown.

See eclipse-platform/eclipse.platform.runtime#35
iloveeclipse added a commit to iloveeclipse/eclipse.platform.runtime that referenced this issue Apr 22, 2022
@iloveeclipse
Added check for not intended default workspace location use (eclipse-…
1ab5d57 
…platform#35)

See original bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=514333
that was only partly fixed.

The original fix added -Dosgi.dataAreaRequiresExplicitInit=true system
property and changed org.eclipse.core.internal.runtime.DataArea in the
way that it doesn't allow to initialize itself if the workspace location
was not explicitly specified yet - either via -data or via workspace
selection prompt.

But the fix in DataArea does not work if bad client code calls
Platform.getLocation() *before* calling Plugin.getStateLocation(),
because Platform.getLocation() doesn't run into changed DataArea code at
all.

Exact this happened in commit
eclipse-platform/eclipse.platform.resources@a8a8d82
where workspace init order was slightly changed: original code
initialized LocalMetaArea first (which then failed as supposed in
assertLocationInitialized() if workspace was not set yet), and after
that initialized WorkspaceRoot (which uses Platform.getLocation() after
the check). Changed code initialized WorkspaceRoot first, and because
this uses Platform.getLocation(), the code silently initializes default
workspace location even if -Dosgi.dataAreaRequiresExplicitInit=true is
set.

The current fix makes sure that if the system flag is given, we disallow
silent workspace location initialization by adding similar check in
Platform.getLocation().

Fixes issue
eclipse-platform#35
iloveeclipse added a commit that referenced this issue Apr 22, 2022
@iloveeclipse
Added check for not intended default workspace location use (#35)
c45ef62 
See original bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=514333
that was only partly fixed.

The original fix added -Dosgi.dataAreaRequiresExplicitInit=true system
property and changed org.eclipse.core.internal.runtime.DataArea in the
way that it doesn't allow to initialize itself if the workspace location
was not explicitly specified yet - either via -data or via workspace
selection prompt.

But the fix in DataArea does not work if bad client code calls
Platform.getLocation() *before* calling Plugin.getStateLocation(),
because Platform.getLocation() doesn't run into changed DataArea code at
all.

Exact this happened in commit
eclipse-platform/eclipse.platform.resources@a8a8d82
where workspace init order was slightly changed: original code
initialized LocalMetaArea first (which then failed as supposed in
assertLocationInitialized() if workspace was not set yet), and after
that initialized WorkspaceRoot (which uses Platform.getLocation() after
the check). Changed code initialized WorkspaceRoot first, and because
this uses Platform.getLocation(), the code silently initializes default
workspace location even if -Dosgi.dataAreaRequiresExplicitInit=true is
set.

The current fix makes sure that if the system flag is given, we disallow
silent workspace location initialization by adding similar check in
Platform.getLocation().

Fixes issue
#35
@laeubi
Copy link
Contributor

laeubi commented Apr 23, 2022

Is osgi.dataAreaRequiresExplicitInit something I would use for debugging? Or is it expsected that users set this or defined by EEPs?

@iloveeclipse
Copy link
Member Author

Is osgi.dataAreaRequiresExplicitInit something I would use for debugging?

Not really.

Or is it expsected that users set this or defined by EEPs?

It is expected to be set by the final product that expectes user to select the workspace location and to make sure no code would init workspace to some (unexpected by user) default location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@iloveeclipse iloveeclipse

Labels
bug Something isn't working regression
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@iloveeclipse @laeubi