Add a possibility to get a dynamic lobrary location for wxDynamicLibrary::Load() (PR #24052)
oneeyeman1
Please review and apply.
I added couple of comments to explain the reasoning.
And sorry for too many commits.
I will add the documentation after the good review, unless directed otherwise.
Thank you.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/24052
Commit Summary
- d2b9bc2 Update the fork with the new changes
- 4a0ea36 Merge branch 'wxWidgets:master' into master
- a045b9a Merge remote-tracking branch 'upstream/master'
- a4f8d10 Merge branch 'master' of github.com:oneeyeman1/wxwidgets
- 230bef1 Implement the feature for GTK
- aa3e924 Implement OSX variant
- f4919fc Implement base version
- d0df031 Added the comments
- 99f64fe Adding some comments for GTK
- 04f6a99 Windows implementation
- 9086525 Fix OSX implementation
- 0fe94c9 Fix GTK implementation
- 3cc156e Adding test for the new function
- a2aa067 Fix typo
- c923f52 Remove the GTK build files
File Changes
(13 files)
- M 3rdparty/pcre (2)
- M include/wx/msw/stdpaths.h (1)
- M include/wx/osx/cocoa/stdpaths.h (2)
- M include/wx/stdpaths.h (4)
- M include/wx/unix/stdpaths.h (1)
- M src/common/stdpbase.cpp (5)
- M src/msw/stdpaths.cpp (4)
- M src/osx/cocoa/stdpaths.mm (9)
- M src/stc/lexilla (2)
- M src/stc/scintilla (2)
- M src/tiff (2)
- M src/unix/stdpaths.cpp (10)
- M tests/interactive/output.cpp (1)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/24052.patch
- https://github.com/wxWidgets/wxWidgets/pull/24052.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- e8b3820 Fix compilation error
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 3 commits.
- f8e9ee5 Remove outdated macro
- 89abcb3 Merge branch 'sharedlibrary' of github.com:oneeyeman1/wxWidgets into sharedlibrary
- 6585f27 Return proper value for Windows
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
I don't understand those failures.
They are completely unrelated to the commits I pushed and I didn't get them locally.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -365,4 +365,16 @@ wxStandardPaths::MakeConfigFileName(const wxString& basename,
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ char c;
+ wxFileName::GetPathSeparator().GetAsChar( &c );
+ // While debugging libraries usually goes to "/usr/local/lib"
+ // while in production the usual place is "/usr/lib"
+#ifdef DEBUG
DEBUG is not used nor defined anywhere.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I don't understand those failures. They are completely unrelated to the commits I pushed
Of course they're related to the submodules that you (wrongly) updated.
I don't understand, do you know you can look at your changes, even after submitting them (it's better to do it before, but...)? Can't you see that the submodules have been changed? Don't you wonder if this is normal/intentional? I'm just lost here...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@vadz ,
I don't understand those failures. They are completely unrelated to the commits I pushed
Of course they're related to the submodules that you (wrongly) updated.
I didn't update them.
After doing "git fetch origin && git merge origin/master" I had the scintilla submodule unmerged. I thought it will take care of everything, but it didn't.
So I deleted src/stc/scintilla and ran the "git submodule update --init".
Please check the thread on the wx-dev and reply from Maarten..
I don't now why it happened, but after doing it I guess everything was updated to its latest version.
Any idea how to fix it easily?
If not - I will probably have to redo the fork and resubmit.
I don't understand, do you know you can look at your changes, even after submitting them (it's better to do it before, but...)? Can't you see that the submodules have been changed? Don't you wonder if this is normal/intentional? I'm just lost here...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Of course they're related to the submodules that you (wrongly) updated.
I didn't update them.
You did update them. The fact that you didn't realize that you updated them is somewhat understandable but the fact that you didn't even look at your changes before pressing "Submit" in spite of being asked to do this literally dozens of time in the past is not. And the fact that you still can somehow claim that you didn't even after I told that you did is just mind boggling.
Again, anybody makes mistakes. But nobody else refuses not only to check for them before submitting but even refuses to see them after submitting. It just drives me crazy that you can look at the compilation errors in the submodules after pushing an update to them and your only conclusion is that something unrelated must be wrong -- and not that you have actually broken it.
You really need to carefully review your next submission, whatever it is, at least twice before submitting it because this colossal waste of time just can't continue any longer.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
Like I said in wx-dev, your branch has diverged from the wxWidgets branch. I'm 99.9% sure you committed wrong submodules to it.
So even git submodule update will update the submodules to that version, not the one in wxWidgets master.
Like I said in wx-dev, you should have done git reset --hard origin/master to get rid of it. You can still do this, but you'll loose the commits with the changes of this PR. But they're visible here on GitHub, so just manually update the files again, commit, and then force push.
And like @vadz said, just click the changed files tab before creating the PR. You would have seen all of those submodules change. And the lobrary typo in the title.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
Like I said in wx-dev, your branch has diverged from the wxWidgets branch. I'm 99.9% sure you committed wrong submodules to it. So even
git submodule updatewill update the submodules to that version, not the one in wxWidgets master.
I don't know how it was possible.
Like I said - all I did is to kill the scintilla folder and ran the "git submidule updaye --init", which should have picked the correct commit from the appropriate submodule.
But that's another mistery that I don't have time to solve right now.
Like I said in wx-dev, you should have done
git reset --hard origin/masterto get rid of it. You can still do this, but you'll loose the commits with the changes of this PR. But they're visible here on GitHub, so just manually update the files again, commit, and then force push.
I can do that command on my PR branch, right?
And like @vadz said, just click the
changed filestab before creating the PR. You would have seen all of those submodules change. And thelobrarytypo in the title.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
I don't know how it was possible. Like I said - all I did is to kill the scintilla folder and ran the "git submidule updaye --init", which should have picked the correct commit from the appropriate submodule.
You could have committed those old submodules weeks or months ago. Not to this branch, but in your master branch. It is not related to the scintilla stuff you did.
I can do that command on my PR branch, right?
Yes.
git reset --hard origin/master
git update submodules
Then add all the changes from https://github.com/wxWidgets/wxWidgets/pull/24052/files and commit them
git push -f
And because your master branch is diverged, also do the first two commands in that branch. Not now, but definitely before you check out a new branch. Otherwise we'll have the same problem all over again.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dan Gudmundsson
git reset --hard origin/master
That depends on what his origin is set to, shouldn't that be upstream or some other name, don't know what you recommend.
But origin is normally his fork of wxWidgets not the wxWidgets repo.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
True, I messed up there. It should be upstream/master. Or whatever name was used for the upstream remote.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -365,4 +365,16 @@ wxStandardPaths::MakeConfigFileName(const wxString& basename,
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ char c;
+ wxFileName::GetPathSeparator().GetAsChar( &c );
+ // While debugging libraries usually goes to "/usr/local/lib"
+ // while in production the usual place is "/usr/lib"
+#ifdef DEBUG
@vadz,
It looks like there is o definition I can use on Linux/Mac with configure build.
Neither DEBUG, not DEBUG_INFO or wxUSE_DEBUG{_INFO} are defined
Any suggestions?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 61fde9c Fixing errors with bad merge
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 3287089 Fixing building documentation
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -365,4 +365,15 @@ wxStandardPaths::MakeConfigFileName(const wxString& basename,
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ // While debugging libraries usually goes to "/usr/local/lib"
+ // while in production the usual place is "/usr/lib"
+#ifdef wxUSE_DEBUG
Using compile definitions and hardcoded paths seem wrong and ununsable to me.
Have you seen GetInstallPrefix()?
> @@ -139,4 +139,13 @@ ConfigFileConv WXUNUSED(conv)) const
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ // Shared libraries on OSX should be stored inside the
+ // <Bundle.app>/Contents/Frameworks
+ wxFileName fn( GetExecutablePath() );
+ wxString temp = fn.GetPath() + wxFileName::GetPathSeparator() + ".." + wxFileName::GetPathSeparator() + "Frameworks" + wxFileName::GetPathSeparator();
This will have .. in the result. There should be some way to resolve this path I assume.
> @@ -367,4 +367,10 @@ wxString wxStandardPathsWin16::GetUserConfigDir() const
return wxGetHomeDir();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ wxFileName fn( GetExecutablePath() );
+ return fn.GetPath() + wxFileName::GetPathSeparator();
Have you seen wxFileName::GetPathWithSep()?
Also relevant in other places.
> @@ -407,6 +407,17 @@ class wxStandardPaths
*/
virtual wxString GetUserLocalDataDir() const;
+ /**
+ Return OS specific directory where proect shared liraries are
proect?
> @@ -407,6 +407,17 @@ class wxStandardPaths
*/
virtual wxString GetUserLocalDataDir() const;
+ /**
+ Return OS specific directory where proect shared liraries are
+
+ - Windows: @c "C:\Programs\AppFolder\"
+ - Unix: @c either /usr/lib/ or /usr/local/lib/
+ - OSX: @c "/Applications/exename.app/Contents/Frameworks/"
Maybe clarify that these are examples. I can install somewhere else on Windows. Or run the macOS .app from a different directory.
And maybe use appinfo instead of exename, like some other documentation does.
> @@ -257,7 +257,7 @@ class WXDLLIMPEXP_BASE wxStandardPaths : public wxStandardPathsBase
{
return m_prefix + wxS("/") + basename;
}
-
+ virtual wxString GetSharedLibraroesPath() const { return m_prefix; }
what is this?
In include/wx/osx/cocoa/stdpaths.h:
> @@ -35,7 +35,7 @@ class WXDLLIMPEXP_BASE wxStandardPaths : public wxStandardPathsBase
virtual wxString MakeConfigFileName(const wxString& basename,
ConfigFileConv conv = ConfigFileConv_Ext
) const override;
-
+ virtual wxString GetSharedLibrariesDir() const override;
For readability, don't replace the empty line, just add a new line.
Also in other places.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -365,4 +365,15 @@ wxStandardPaths::MakeConfigFileName(const wxString& basename,
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ // While debugging libraries usually goes to "/usr/local/lib"
+ // while in production the usual place is "/usr/lib"
+#ifdef wxUSE_DEBUG
@MaartenBent ,
Wow! Thank you for pointing that out. I didn't know about this one. I will look at that and see what it does.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 7800102 Implement PR suggestion
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -139,4 +139,13 @@ ConfigFileConv WXUNUSED(conv)) const
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ // Shared libraries on OSX should be stored inside the
+ // <Bundle.app>/Contents/Frameworks
+ wxFileName fn( GetExecutablePath() );
+ wxString temp = fn.GetPath() + wxFileName::GetPathSeparator() + ".." + wxFileName::GetPathSeparator() + "Frameworks" + wxFileName::GetPathSeparator();
@MaartenBent ,
Fixed in the upcoming commit. Thx.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- bb406e3 Implement PR suggestion
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 2 commits.
- 4561f6f Implement PR suggestion
- 5e8acf3 Merge branch 'sharedlibrary' of github.com:oneeyeman1/wxWidgets into sharedlibrary
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -139,4 +139,17 @@ ConfigFileConv WXUNUSED(conv)) const
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ // Shared libraries on OSX should be stored inside the
+ // <Bundle.app>/Contents/Frameworks
+ wxFileName fn( GetExecutablePath() );
+ auto temp = fn.GetPath();
+ auto pos = temp.rfind( "/MacOS" );
+ if( pos != wxString::npos )
I was thinking of using wxFileName::RemoveLastDir(). You seem to come up with the hardest solutions for a simple problem.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -367,4 +367,10 @@ wxString wxStandardPathsWin16::GetUserConfigDir() const
return wxGetHomeDir();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ wxFileName fn( GetExecutablePath() );
+ return fn.GetPath() + wxFileName::GetPathSeparator();
@MaartenBent ,
Didnt know about that. Fixed. Thx.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -139,4 +139,17 @@ ConfigFileConv WXUNUSED(conv)) const
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ // Shared libraries on OSX should be stored inside the
+ // <Bundle.app>/Contents/Frameworks
+ wxFileName fn( GetExecutablePath() );
+ auto temp = fn.GetPath();
+ auto pos = temp.rfind( "/MacOS" );
+ if( pos != wxString::npos )
On second thought, you probably should use a similar solution as GetExecutablePath().
https://developer.apple.com/documentation/foundation/nsbundle/1408900-builtinpluginspath
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 5e98d52 Fix typo in the new function and another typo
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -407,6 +407,17 @@ class wxStandardPaths
*/
virtual wxString GetUserLocalDataDir() const;
+ /**
+ Return OS specific directory where proect shared liraries are
@MaartenBent ,
Fixed. Also fixed another typo.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -257,7 +257,7 @@ class WXDLLIMPEXP_BASE wxStandardPaths : public wxStandardPathsBase
{
return m_prefix + wxS("/") + basename;
}
-
+ virtual wxString GetSharedLibraroesPath() const { return m_prefix; }
No, this is an entire different function.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -257,7 +257,7 @@ class WXDLLIMPEXP_BASE wxStandardPaths : public wxStandardPathsBase
{
return m_prefix + wxS("/") + basename;
}
-
+ virtual wxString GetSharedLibraroesPath() const { return m_prefix; }
@MaartenBent ,
It is generic implementation.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lauri Nurmi
@lanurmi commented on this pull request.
> @@ -407,6 +407,17 @@ class wxStandardPaths
*/
virtual wxString GetUserLocalDataDir() const;
+ /**
+ Return OS specific directory where proect shared liraries are
+
+ - Windows: @c "C:\Programs\AppFolder\"
+ - Unix: @c either /usr/lib/ or /usr/local/lib/
+ - OSX: @c "/Applications/exename.app/Contents/Frameworks/"
What version of Windows uses literally C:\Programs?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -407,6 +407,17 @@ class wxStandardPaths
*/
virtual wxString GetUserLocalDataDir() const;
+ /**
+ Return OS specific directory where proect shared liraries are
+
+ - Windows: @c "C:\Programs\AppFolder\"
+ - Unix: @c either /usr/lib/ or /usr/local/lib/
+ - OSX: @c "/Applications/exename.app/Contents/Frameworks/"
@lanurmi ,
I don't know. But that's what the documentation says.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -139,4 +139,17 @@ ConfigFileConv WXUNUSED(conv)) const
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ // Shared libraries on OSX should be stored inside the
+ // <Bundle.app>/Contents/Frameworks
+ wxFileName fn( GetExecutablePath() );
+ auto temp = fn.GetPath();
+ auto pos = temp.rfind( "/MacOS" );
+ if( pos != wxString::npos )
On second thought, you probably should use a similar solution as
GetExecutablePath(). https://developer.apple.com/documentation/foundation/nsbundle/1409078-executablepath
That what OSX is using inside GetExecutablePath().
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -139,4 +139,17 @@ ConfigFileConv WXUNUSED(conv)) const
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ // Shared libraries on OSX should be stored inside the
+ // <Bundle.app>/Contents/Frameworks
+ wxFileName fn( GetExecutablePath() );
+ auto temp = fn.GetPath();
+ auto pos = temp.rfind( "/MacOS" );
+ if( pos != wxString::npos )
I was thinking of using
wxFileName::RemoveLastDir(). You seem to come up with the hardest solutions for a simple problem.
I used the solution from the GetInstallPrefix().
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 2 commits.
- 92739be Implement PR suggestion
- c9adf09 Merge branch 'sharedlibrary' of github.com:oneeyeman1/wxwidgets into sharedlibrary
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -139,4 +139,17 @@ ConfigFileConv WXUNUSED(conv)) const
return fn.GetFullName();
}
+wxString wxStandardPaths::GetSharedLibrariesDir() const
+{
+ // Shared libraries on OSX should be stored inside the
+ // <Bundle.app>/Contents/Frameworks
+ wxFileName fn( GetExecutablePath() );
+ auto temp = fn.GetPath();
+ auto pos = temp.rfind( "/MacOS" );
+ if( pos != wxString::npos )
@MaartenBent
After thinking more about it I changed it to use your latest suggestion.
Thx.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@MaartenBent ,
Do you think its now ready?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lauri Nurmi
@lanurmi commented on this pull request.
> @@ -407,6 +407,17 @@ class wxStandardPaths
*/
virtual wxString GetUserLocalDataDir() const;
+ /**
+ Return OS specific directory where proect shared liraries are
+
+ - Windows: @c "C:\Programs\AppFolder\"
+ - Unix: @c either /usr/lib/ or /usr/local/lib/
+ - OSX: @c "/Applications/exename.app/Contents/Frameworks/"
@lanurmi , I don't know. But that's what the documentation says.
I can't find any search results for C:\Programs either in 'documentation' or anywhere else on the web.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@oneeyeman1 no, there are still open issues, see all the unresolved comments. Here my comments after a code review, I didn't test it so I assume it at least works.
- There is a
GetSharedLibraroesPath, which has nothing to do withGetSharedLibrariesDir. - On Windows you should move the implementation, it is in the wxStandardPathsWin16 section now. And maybe you could use
GetAppDir, just likeGetPluginsDirdoes, see also the comment in that function. - On macOS you should use the designated API, not
GetExecutablePath. - The documentation needs updating, see the comments. The Windows location is very unrealistic as @lanurmi commented. Shouldn't it be Linux instead of Unix? And the OSX (is it still called that? probably should use macOS) name can be improved. Maybe make them relative to a install path instead of using fixed paths.
- Less important, but if you look at the diff you can see that you replaced empty lines, you should add the declarations on new lines. It would be nice if you could fix this so @vadz doesn't have to do all the cleanup and can just squash and merge.
And a general question. Is the location in Linux always /lib, or could it also be /lib64. I suppose it depends on where the packagers decide to install it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
There is a GetSharedLibraroesPath, which has nothing to do with GetSharedLibrariesDir.
This will be fixed in an upcoming commit.
On macOS you should use the designated API, not GetExecutablePath.
According to Apple docs the folder is called Frameworks. And that'ss where Xcode places the dynamic libraries when it creates Bundle.
Using that API I get the folder called SharedFramework.
The documentation needs updating, see the comments. The Windows location is very unrealistic as @lanurmi commented. Shouldn't it be Linux instead of Unix? And the OSX (is it still called that? probably should use macOS) name can be improved. Maybe make them relative to a install path instead of using fixed paths.
Looking at the documentation of GetExecutablePath(), it uses Unix, not Linux, but it does use Mac and not OSX. Will fix. Also I will change the Windows verbiage to explain what will happen.
And a general question. Is the location in Linux always /lib, or could it also be /lib64. I suppose it depends on where the packagers decide to install it.
I think one is a symlink ro another.
It is the case on my home machine. And it is mute, since I changed the verbiage for it to make it better.
On Windows you should move the implementation, it is in the wxStandardPathsWin16 section now. And maybe you could use GetAppDir, just like GetPluginsDir does, see also the comment in that function.
I will check the Windows and make sure its fixed as well.
I did look and I didn't find any additional conditional directives inside src/msw/stdpaths.cpp
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 1366cbb Fix documentation typos
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 28b3751 Implement PR suggestion
You are receiving this because you are subscribed to this thread.
Ian McInerney
I think one is a symlink ro another.
It is the case on my home machine. And it is mute, since I changed the verbiage for it to make it better.
It is not guaranteed to be a symlink - on Fedora /usr/lib and /usr/lib64 are two different directories. And the verbage would be wrong on Fedora, because the libraries are installed into /usr/lib64 and you are not returning that directory.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
I think one is a symlink ro another.
It is the case on my home machine. And it is mute, since I changed the verbiage for it to make it better.It is not guaranteed to be a symlink - on Fedora /usr/lib and /usr/lib64 are two different directories. And the verbage would be wrong on Fedora, because the libraries are installed into
/usr/lib64and you are not returning that directory.
Did you try it?
What it returns for you?
And what GetInstallPrefix() returns for you?
Thx.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -257,7 +257,7 @@ class WXDLLIMPEXP_BASE wxStandardPaths : public wxStandardPathsBase
{
return m_prefix + wxS("/") + basename;
}
-
+ virtual wxString GetSharedLibraroesPath() const { return m_prefix; }
No, this is an entire different function.
Sorry, after looking at the code more close it appears to be a function that will be used if generic verion of the class is used.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@MaartenBent ,
Also I just re-checked and wxStandardPathsWin16 does not contain an implementation of my function:
[code]
Igors-MacBook-Air:wxFork igorkorot$ grep -riI wxStandardPathsWin16 *
include/wx/msw/stdpaths.h:// wxStandardPathsWin16: this class is for internal use only
include/wx/msw/stdpaths.h:class WXDLLIMPEXP_BASE wxStandardPathsWin16 : public wxStandardPaths
patch:@@ -367,4 +367,9 @@ wxString wxStandardPathsWin16::GetUserConfigDir() const
src/msw/stdpaths.cpp:// wxStandardPathsWin16 implementation
src/msw/stdpaths.cpp:wxString wxStandardPathsWin16::GetConfigDir() const
src/msw/stdpaths.cpp:wxString wxStandardPathsWin16::GetUserConfigDir() const
Igors-MacBook-Air:wxFork igorkorot$
[/code]
Thank you.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@MaartenBent ,
I have been using this code for about a month and didn't see anything bad.
@vadz ,
Should I update the documentation @SInCE tag?
Could you please look at it?
Thank you.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
Also I just re-checked and wxStandardPathsWin16 does not contain an implementation of my function
I don't mean the class, just the location where you put the code:
Screenshot.2023-11-25.134827.png (view on web)
Do you see the difference between these two lines?
GetSharedLibrariesDir
GetSharedLibraresDir
Check your code because it is in there. The line with the typo should have an override keyword so the compiler will throw an error that you are trying to override a function that does not exist.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@MaartenBent ,
Could you please check what else is missing?
Thx
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
The typo in GetSharedLibraresDir is still there
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- e36a757 Fix the spelling f the function
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
The typo in
GetSharedLibraresDiris still there
Fixed in the latest commit
Can you apply it?
Thx
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
You replaced one typo with another: overrde
And override and const are in the wrong order.
I suggest to actually test this code. Normally it is not enabled, see the note above the class definition. You can enable it with wxUSE_STDPATHS=0 (when using CMake or the MSVC solution) ,or (I think) --disable-stdpaths with configure.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 5794f0c Fixing the typo; Fix compilation error on MSW when USE_STDPATH 0
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@MaartenBent ,
I fixed the typo and also fixed the error message on Windows when USE_STDPATH is 0.
However, in this case I'm getting following warnings:
c:\wxfork\include\wx\stdpaths.h(262): warning C4100: 'csidl': unreferenced formal parameter (compiling source file ..\..\src\common\docview.cpp)
c:\wxfork\include\wx\stdpaths.h(262): warning C4100: 'csidl': unreferenced formal parameter (compiling source file ..\..\src\msw\rt\notifmsgrt.cpp)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
I see MSWGetShellDir was added back in 2009, and only now we notice it is missing in this wxStandardPaths implementation. So fixing the warning isn't the highest priority.
It should be simple to fix though using WXUNUSED, like is also done for a few other methods in this class.
static wxString MSWGetShellDir(int WXUNUSED(csidl)) { return wxEmptyString; }
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@MaartenBent ,
If I do this I get link error about not finding proper functin.
But as you say - it's not high priority.
Can you push that code?
Thank you.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
I get the error too when building shared libraries. The problem is that __WXMSW__ is not defined for the base library, only the core library.
So I think the best fix is to use #if defined(__WINDOWS__), just like line 221 in the same file.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- b8f328a Fix compilation on MSW when USE_STDPATH 0
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@MaartenBent ,
You are right.. Just verified and compilation finished fine.
Could you also look at #23558?
I put some comments in and added some additional fixes to the text code....
Thx.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@MaartenBent ,
Is this now ready to be merged?
Thx
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
The documentation needs to explain how is the new function different from the existing GetPluginsDir().
It also should either mention that the path is /-terminated or, better, remove it because this seems to be inconsistent with all the other functions and with the empty return value in the base class.
> +#if defined(__WINDOWS__)
+ static wxString MSWGetShellDir(int WXUNUSED(csidl)) { return wxEmptyString; }
+#endif
What's this and why do we need it? It doesn't seem to be used anywhere.
> @@ -171,3 +171,7 @@ wxString wxStandardPathsBase::AppendAppInfo(const wxString& dir) const
return subdir;
}
+wxString wxStandardPathsBase::GetSharedLibrariesDir() const
+{
+ return "";
⬇️ Suggested change
- return "";
+ return {};
> @@ -407,6 +407,17 @@ class wxStandardPaths
*/
virtual wxString GetUserLocalDataDir() const;
+ /**
+ Return OS specific directory where project shared liraries are
+
+ - Windows: @c it returns the folder where the application binary is located
+ - Unix: @c it returns the libraries installation path
This is not really clear, I'd give lib directory as an example.
> + // Shared libraries on OSX should be stored inside the + // <Bundle.app>/Contents/Frameworks + wxFileName fn( GetExecutablePath() ); + fn.RemoveLastDir(); + return fn.GetPathWithSep() + "Frameworks/";
I think we need one of the functions from this page instead although I'm not sure which one. Please try using them to see which of them returns Frameworks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> +#if defined(__WINDOWS__)
+ static wxString MSWGetShellDir(int WXUNUSED(csidl)) { return wxEmptyString; }
+#endif
@vadz,
When you turn off wxUSE_STDPATH and compile on Windows this function has to be defined.
Otherwise you will get compiler error
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- c92ed6d Implement code review suggestions
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -171,3 +171,7 @@ wxString wxStandardPathsBase::AppendAppInfo(const wxString& dir) const
return subdir;
}
+wxString wxStandardPathsBase::GetSharedLibrariesDir() const
+{
+ return "";
Fixed
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -407,6 +407,17 @@ class wxStandardPaths
*/
virtual wxString GetUserLocalDataDir() const;
+ /**
+ Return OS specific directory where project shared liraries are
+
+ - Windows: @c it returns the folder where the application binary is located
+ - Unix: @c it returns the libraries installation path
Clarifiied
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 2 commits.
- a6f9bf8 Use dedicated OS function call
- a146902 Merge branch 'sharedlibrary' of github.com:oneeyeman1/wxwidgets into sharedlibrary
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> + // Shared libraries on OSX should be stored inside the + // <Bundle.app>/Contents/Frameworks + wxFileName fn( GetExecutablePath() ); + fn.RemoveLastDir(); + return fn.GetPathWithSep() + "Frameworks/";
@vadz ,
Found it.
Tested and committed.
Thx
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@vadz ,
The documentation needs to explain how is the new function different from the existing
GetPluginsDir().
I will try to put some wording explaining this.
It also should either mention that the path is
/-terminated or, better, remove it because this seems to be inconsistent with all the other functions and with the empty return value in the base class.
Will remove.
Thx
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 41a2da6 Do not end the function with the separator as no other function in this class does
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 105db62 Fix cmpilation error after previous connit
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 28fc28f Trying to clarify the difference with existing API
You are receiving this because you are subscribed to this thread.
oneeyeman1
@vadz ,
I tried to clarify the difference between those 2 functions.
Feel free to critique my explanation.
Thank you
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> +#if defined(__WINDOWS__)
+ static wxString MSWGetShellDir(int WXUNUSED(csidl)) { return wxEmptyString; }
+#endif
Ah, so this has nothing to do with this PR.
This is not the proper fix for the problem, of course. AFAICS this is only used in src/msw/rt/notifmsgrt.cpp and so the right thing to do would be to add #if wxUSE_STDPATH around the code there. This really has nothing to do here in any case.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, this is close to being mergeable now, just a couple of minor things to fix.
> @@ -143,9 +143,7 @@ ConfigFileConv WXUNUSED(conv)) const
{
// Shared libraries on OSX should be stored inside the
// <Bundle.app>/Contents/Frameworks
- wxFileName fn( GetExecutablePath() );
- fn.RemoveLastDir();
- return fn.GetPathWithSep() + "Frameworks/";
+ return wxCFStringRef::AsString((CFStringRef)[NSBundle mainBundle].privateFrameworksPath);
Why do you need the cast here? Using NSString* directly should work too, i.e.
- return wxCFStringRef::AsString((CFStringRef)[NSBundle mainBundle].privateFrameworksPath); + return wxCFStringRef::AsString([NSBundle mainBundle].privateFrameworksPath);
> + The function is different from GetPluginsDir() as PlugIns + and Shared Libraries are 2 different things (especially on Mac). + Plugin is a library that can be used without restartng the applicatioon + whereas the library needes to be installed and the application restarted.
Thanks, this does explain it, but I'd just say
⬇️ Suggested change- The function is different from GetPluginsDir() as PlugIns - and Shared Libraries are 2 different things (especially on Mac). - Plugin is a library that can be used without restartng the applicatioon - whereas the library needes to be installed and the application restarted. + + The function does the same thing as GetPluginsDir() under non-Mac platforms + but differs from it under Mac, where plugins (shared libraries loaded by the + application dynamically while it's running) and shared libraries (that the + application is statically linked with) are stored in different directories.
>
- Windows: @c it returns the folder where the application binary is located
- - Unix: @c it returns the libraries installation path
+ - Unix: @c it returns the libraries installation path, i.e. /usr/lib
- Mac: @c "/Applications/exename.app/Contents/Frameworks/"
This is wrong now and should be
⬇️ Suggested change- - Mac: @c "/Applications/exename.app/Contents/Frameworks/" + - Mac: @c "/Applications/exename.app/Contents/Frameworks"
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -143,9 +143,7 @@ ConfigFileConv WXUNUSED(conv)) const
{
// Shared libraries on OSX should be stored inside the
// <Bundle.app>/Contents/Frameworks
- wxFileName fn( GetExecutablePath() );
- fn.RemoveLastDir();
- return fn.GetPathWithSep() + "Frameworks/";
+ return wxCFStringRef::AsString((CFStringRef)[NSBundle mainBundle].privateFrameworksPath);
@vadz ,
There is another function that uses this cast, so I just copied it here.
Will check and if its not needed - will remove.
Do you want to fix the other function as well?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> +#if defined(__WINDOWS__)
+ static wxString MSWGetShellDir(int WXUNUSED(csidl)) { return wxEmptyString; }
+#endif
@vadz ,
No it doesn't..
However, when I set wxUSE_STDPATH to 0 and tried to compile with MSVC I got an error.
All I was trying to do is make sure the code compiles. And when I saw it didn't - I tried to fix it.
I can remove this piece of code here and add the ifdef in there in another PR fixing it. However, I will not be able to test it
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> + The function is different from GetPluginsDir() as PlugIns + and Shared Libraries are 2 different things (especially on Mac). + Plugin is a library that can be used without restartng the applicatioon + whereas the library needes to be installed and the application restarted.
@vadz ,
Fixed in the upcoming commit.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
>
- Windows: @c it returns the folder where the application binary is located
- - Unix: @c it returns the libraries installation path
+ - Unix: @c it returns the libraries installation path, i.e. /usr/lib
- Mac: @c "/Applications/exename.app/Contents/Frameworks/"
@vadz ,
Fixed in the upcoming commit.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 1 commit.
- 7a4a8bc Implement code review suggestions
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 pushed 2 commits.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@oneeyeman1 commented on this pull request.
> @@ -143,9 +143,7 @@ ConfigFileConv WXUNUSED(conv)) const
{
// Shared libraries on OSX should be stored inside the
// <Bundle.app>/Contents/Frameworks
- wxFileName fn( GetExecutablePath() );
- fn.RemoveLastDir();
- return fn.GetPathWithSep() + "Frameworks/";
+ return wxCFStringRef::AsString((CFStringRef)[NSBundle mainBundle].privateFrameworksPath);
@vadz ,
I removed the cast, but only for that function.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@vadz ,
I see at least 2 potential PR after this is accepted - remove the cast n all other MAC functions and fix the MSW code.
Could you push this PR as is right now and I'll try to make follow-up same day?
TIA!!!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Finally merging with minor changes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
Thx.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.