Christian :Biesinger (don't email me, ping me on IRC) Reporter
	Description • 19 years ago

The BeOS tinderbox on http://tinderbox.mozilla.org/SeaMonkey-Ports/ is currently red. It needs an nsGlueLinkingBeOS file in xpcom/glue/standalone.

	tqh
	Comment 1 • 19 years ago

I've said I'd take a look at it, but I'm buzy until next week.

	Christian :Biesinger (don't email me, ping me on IRC) Reporter
	Comment 2 • 19 years ago

(btw... comment 0 is wrong I think... it's another bug that causes the redness. still, this bug is valid)

	Sergei Dolgov Assignee
	Comment 3 • 18 years ago

adds BeOS section:
ifeq (BeOS,$(OS_ARCH))
LINKSRC = nsGlueLinkingBeOS.cpp
endif

	Sergei Dolgov Assignee
	Comment 4 • 18 years ago

nsGlueLinkingBeOS.cpp
implements dynamic linking for BeOS via load_add_on.

Nees some attention of tqh and XPCOM developers.
Because, if it is intended to load also huge libs/components, we need here same workaround with *.stub as in prlink.c

	tqh
	Comment 5 • 18 years ago

Ah, well adding stub-loading to that will be easy. I think we should ifdef it to avoid it on Zeta/Haiku.

	Sergei Dolgov Assignee
	Comment 6 • 18 years ago

One thing besides stubs, which may be really critical - that LEADING_UNDERSCORE

I don't know if we need it. It is possible that it rather breaks get_image_symbol 
 functionality at all. B_SYMBOL_TYPE_TEXT must be enough and exhaustive to find functions by name.

But here raises also question - does dlsym/get_image_symbol is intended to find there function names only or arbitrary symbols?
If last, we should replace it with B_SYMBOL_TYPE_ANY.

Waiting for mmadia tinderbox test results on that

	Sergei Dolgov Assignee
	Comment 7 • 18 years ago

mmadia, we need your help here, with compiling it in existing tree and running just that XPCOM tests which turn our tinderbox to orange.

Also, i have to notice, that two more files in 
mozilla/xpcom/glue/standalone
have rich platfrom-dependent content:
nsGREGlue
and
nsGREDirServiceProvider

for WIN, OSX and UNIX but whose are lacking XP_BEOS in ifdefs.
Wondering how it affects potential BeOS-port functionality

	Sergei Dolgov Assignee
	Comment 8 • 18 years ago

adding BeOS case  to Unix in nsGREDirServiceProvider.cpp

	Sergei Dolgov Assignee
	Comment 9 • 18 years ago

yet another file to fix:
xpcom/glue/nsGREGlue.cpp

	Sergei Dolgov Assignee
	Comment 10 • 18 years ago

nsGREGlue - adding BeOS

	Sergei Dolgov Assignee
	Comment 11 • 18 years ago

bit updated version. No need to create entry for normalization
r=?

	Sergei Dolgov Assignee
	Comment 12 • 18 years ago

Comment on attachment 225087 [details] [diff] [review]
nsGREGlue patch

r=?

	Sergei Dolgov Assignee
	Comment 13 • 18 years ago

Comment on attachment 225065 [details] [diff] [review]
nsGREDirServiceProvider patch

obsoleting. There may be better was for beos.
By using image GetInfo()

	Sergei Dolgov Assignee
	Comment 14 • 18 years ago

Sets highest priority to actual path of running executable.

Sets environment according to that path for other components need
(this is questionable if method
GRE_GetCurrentProcessDirectory(char* buffer) is called too frequently. Maybe putenv will decrease performance in such case, but maybe it don't. Maybe bsmedberg knows?)

And only then, in unbelievable case of get_next_image_info() failure, it uses unixish fallback way.

	Sergei Dolgov Assignee
	Comment 15 • 18 years ago

Comment on attachment 224736 [details] [diff] [review]
makefile.in patch

r=?

	tqh
	Comment 16 • 18 years ago

Comment on attachment 225087 [details] [diff] [review]
nsGREGlue patch

r=thesuckiestemail@yahoo.se
Nitpick: I prefer NULL over 0 as NULL states 'nothing' but 0 is also the value zero.

	tqh
	Comment 17 • 18 years ago

Comment on attachment 225090 [details]
nsGlueLinkingBeOS.cpp file ver 2

r=thesuckiestemail@yahoo.se

	tqh
	Comment 18 • 18 years ago

Comment on attachment 225099 [details] [diff] [review]
nsGREDirServiceProvider second patch

The comment could be clearer and some mixed style in that code:

 if( vs if (
if (a) vs if (b == ns_null)

	Sergei Dolgov Assignee
	Comment 19 • 18 years ago

fallback removed, unneeded headers removed.

	Sergei Dolgov Assignee
	Comment 20 • 18 years ago

Comment on attachment 225087 [details] [diff] [review]
nsGREGlue patch

as this file is common for all platforms, asking for second look by peer.

	tqh
	Comment 21 • 18 years ago

Comment on attachment 225117 [details] [diff] [review]
patch - next version for nsGREDirServiceProvider

r=thesuckiestemail@yahoo.se

	Sergei Dolgov Assignee
	Comment 22 • 18 years ago

Comment on attachment 225117 [details] [diff] [review]
patch - next version for nsGREDirServiceProvider

Common for all platforms file, so asking additional review for safety.

	Doug Shelton
	Comment 23 • 18 years ago

In testing, I pulled latest nsGREGlue.cpp from CVS and applied patch, above.  Resulting file does not build under Zeta.  

Partial error message:
/boot/home/develop/mozilla/xpcom/glue/nsGREGlue.cpp: In function `nsresult GRE_GetGREPathWithProperties(const GREVersionRange *, unsigned int, const GREProperty *, unsigned int, char *, unsigned int)':
/boot/home/develop/mozilla/xpcom/glue/nsGREGlue.cpp:291: parse error before `,'
/boot/home/develop/mozilla/xpcom/glue/nsGREGlue.cpp:310: `GRE_CONF_DIR' undeclared (first use this function) 

	Sergei Dolgov Assignee
	Comment 24 • 18 years ago

looks strange, compiled that file here and made patch for this.
Will look if something was changed inbetween

	Sergei Dolgov Assignee
	Comment 25 • 18 years ago

ok, we need probably some changes also in nsXPCOMPrivate.h BeOS section

	Sergei Dolgov Assignee
	Comment 26 • 18 years ago

Comment on attachment 225087 [details] [diff] [review]
nsGREGlue patch

need to cleanup things with includes first, so it will be common patch for XPCOmPrivate and GLue

	Sergei Dolgov Assignee
	Comment 27 • 18 years ago

Problem here if GRE_CONF_DIR  and GRE_USER_CONF_DIR should be really different in BeOS.
We aren't multiuser, though. nothing prevents from having 
GRE_USER_CONF_DIR ".gre.d"

	Sergei Dolgov Assignee
	Comment 28 • 18 years ago

Includes all previous patches as is + additional defines in nsXPCOMPrivate.h

For module owner or peer review convinience.

	Sergei Dolgov Assignee
	Comment 29 • 18 years ago

Comment on attachment 225199 [details] [diff] [review]
full patch

Asking review for files which are commons for all platforms.
Changes in patch are BeOS-specific and also reviewed previously by BeOS developer.

	Benjamin Smedberg
	Comment 30 • 18 years ago

Comment on attachment 225199 [details] [diff] [review]
full patch

>Index: mozilla/xpcom/build/nsXPCOMPrivate.h

>+// That's hack. Until all places which use those defs are rewritten to use find_directory()
>+// And more, lot of things may be rewritten using queries, extended attributes and app signature.

I don't understand this comment at all. Please make it a TODO: comment with a clearer explanation of what should happen.

>+#define GRE_CONF_PATH "/boot/home/config/settings/GRE/GRE/gre.conf"
>+#define GRE_CONF_DIR  "/boot/home/config/settings/GRE/gre.d"

This doesn't look right. I suspect you want
#define GRE_CONF_PATH "/boot/home/config/settings/GRE/gre.conf
#define GRE_CONF_DIR  "/boot/home/config/settings/GRE/gre.d"

>+// We aren't multiuser at the moment.
>+#define GRE_USER_CONF_DIR GRE_CONF_DIR

I don't think you want to define GRE_USER_CONF_DIR at all in this case, or else you'll end up reading the same files twice. (And make sure to ifdef out the code in nsGREGlue.cpp which uses GRE_USER_CONF_DIR.)

	Sergei Dolgov Assignee
	Comment 31 • 18 years ago

according to BeWitched tinderbox
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1153708620.21722.gz
it busted BeOS builds.
Will prepare new version of patch

	Sergei Dolgov Assignee
	Comment 32 • 18 years ago

Separated standalone patch to fix bustage. Those parts were already reviewed by BeOS port developer, so it is actually second review/MOA request.

(XPCOM/glue/. changes which were rejected by bsmedberg are independent and will be fixed later)

	Sergei Dolgov Assignee
	Comment 33 • 18 years ago

Comment on attachment 225090 [details]
nsGlueLinkingBeOS.cpp file ver 2

File is BeOS-specific, but
 lives in non-BeOS folder.
So formal second review request.

	Benjamin Smedberg
	Comment 34 • 18 years ago

Comment on attachment 225090 [details]
nsGlueLinkingBeOS.cpp file ver 2

Why don't you just skip the whole LEADING_UNDERSCORE bit, since you know that you don't have a leading underscore on BeOS?

	Sergei Dolgov Assignee
	Comment 35 • 18 years ago

per comment 34:
Sure, no need for underscore ifdefs atm.
I left it there for historical, or vice versa, future reason. There was BeOS PPC with Metrowerks compiler where it was different.
Sure, no BeOS PPC Mozilla port atm, but open-source version of BeOS (with name Haiku OS - and Mozilla works there already at x86) may support several hardware platforms and compilers.

	Sergei Dolgov Assignee
	Comment 36 • 18 years ago

Checking in mozilla/xpcom/glue/standalone/Makefile.in;
/cvsroot/mozilla/xpcom/glue/standalone/Makefile.in,v  <--  Makefile.in
new revision: 1.31; previous revision: 1.30
done
Checking in mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp;
/cvsroot/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp,v  <--  nsGREDirServiceProvider.cpp
new revision: 1.44; previous revision: 1.43
done
RCS file: /cvsroot/mozilla/xpcom/glue/standalone/nsGlueLinkingBeOS.cpp,v
done
Checking in mozilla/xpcom/glue/standalone/nsGlueLinkingBeOS.cpp;
/cvsroot/mozilla/xpcom/glue/standalone/nsGlueLinkingBeOS.cpp,v  <--  nsGlueLinkingBeOS.cpp
initial revision: 1.1
done 

	Sergei Dolgov Assignee
	Comment 37 • 18 years ago

bustage should be fixed for now, remaining part isn't CRITICAL

	Sergei Dolgov Assignee
	Comment 38 • 18 years ago

next take.
Separate patch for GREGlue.
Now in more BeOS-ish way, no hardcoded paths, using find_directory() for B_USER and B_COMMON SETTINGS_DIR instead.

	tqh
	Comment 39 • 18 years ago

Comment on attachment 230647 [details] [diff] [review]
patch for GREGlue

Shouldn't there be a #elif here: https://bugzilla.mozilla.org/attachment.cgi?id=230647&action=diff#mozilla/xpcom/glue/nsGREGlue.cpp_sec3

Seems the patchfile is broken, the line looks like this in raw format:
++#elif XP_BEOS

Havn't looked closer because of this.

	Sergei Dolgov Assignee
	Comment 40 • 18 years ago

removed extra + before #elif

	Sergei Dolgov Assignee
	Comment 41 • 18 years ago

With coming multiuser support in Zeta
http://joomla.iscomputeron.com/index.php?option=com_content&task=view&id=890
severity if this and some other bugs is increasing

We need to get rid of hardcoded, especially absolute, "char *" paths where it is possible and as soon as possible.

In nsXPCOMPrivate.h, in nspr and nss.

	tqh
	Comment 42 • 18 years ago

Comment on attachment 230725 [details] [diff] [review]
patch v2 for GREGlue

r=thesuckiestemail@yahoo.se

	Sergei Dolgov Assignee
	Comment 43 • 18 years ago

Comment on attachment 230725 [details] [diff] [review]
patch v2 for GREGlue

second review request

	Sergei Dolgov Assignee
	Comment 44 • 18 years ago

Checking in mozilla/xpcom/build/nsXPCOMPrivate.h;
/cvsroot/mozilla/xpcom/build/nsXPCOMPrivate.h,v  <--  nsXPCOMPrivate.h
new revision: 1.43; previous revision: 1.42
done
Checking in mozilla/xpcom/glue/nsGREGlue.cpp;
/cvsroot/mozilla/xpcom/glue/nsGREGlue.cpp,v  <--  nsGREGlue.cpp
new revision: 1.20; previous revision: 1.19
done 

Wondering if 1.8* branch needs this fix as well

	Niels Reedijk
	Comment 45 • 18 years ago

Sergei, what merits does this patch have? I can't really tell from the bug report what improvements it might offer for Firefox 2. If you could describe that in one or two lines, I think we can consider whether it should be in FF2. 

	Sergei Dolgov Assignee
	Comment 46 • 18 years ago

Niels, there is no actual rush with that. At least for FF/SM branch releases for BeOS.

This is directed to support new incarnation of Gecko Runtime Engine for various application, as far as I understood and allows it work independently of those traditional components which are bootstrapping app and shared libraries.

And if and we decide we need it for some purposes in branch, first thing we should look if other platforms really have it implemented in that branch - I'm unsure about it atm. Maybe we look at that when back from Sweden.