Closed Bug 625895 Opened 14 years ago Closed 9 years ago

Build standalone-JS with embedded NSPR

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 975011

People

(Reporter: wes, Assigned: wes)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch to remove --enable-threadsafe and automatically embed NSPR unless other options are given
5.92 KB, patch
jimb
: review-
Details | Diff | Splinter Review
The upcoming removal of --enable-threadsafe (subsequent hard requirement for NSPR) complicates the JS building story. This patch allows us to build SpiderMonkey from a tracemonkey/m-c/etc tree without having an already-installed NSPR or spending time fiddling with config parameters. We build NSPR in the SpiderMonkey build directory, and link the object files right into libmozjs, js_static, and the js shell. The linker decides which bits of NSPR to throw out. We simply reuse the vast majority of the NSPR build infrastructure and let the linker decide what objects it doesn't need. This may seem a little ugly, but the alternative is significantly uglier and much harder to maintain, due to a high degree of inter-object dependencies in NSPR which vary from platform to platform.
Attachment #503971 - Flags: review?(jimb)
So now there are three ways to get NSPR? This is bad, but I agree that having an embedded option is desirable. The error message could be better: "Please use only one of emabedded/system/cflags" for example. Which should be the default? Right now there are 4 options: none, cflags/libs, system, and embedded. I'm torn between: - system being default, use embedded if system is unavailable - The advantage here is a lower default compile-time. - embedded being default, use system only if asked for it. - The advantage here is that everyone is in a homogenous environment. Not that big of a deal though, considered NSPR's stability. I think I'm slightly in favour of the former.
A non-parallel make on my vanilla iMac shows this patch costs about 14 seconds of build time, including autoconf. Building with system NSPR resembles what will make it into Linux distros (operations like Debian do not allow embedded libraries) Building with embedded NSPR more strongly resembles a browser build. Building with system NSPR has a minor gotcha - if the distro's NSPR is too old, your build fails during the configuration phase. Embedded NSPR is guaranteed to always work. What should be default? Depends if your goal is a fast initial build or ensuring correctness/browser-behaviour. As for multiple ways to specify dependent libs - the fact is that sometimes you need to link with your distro's libs, sometimes you need to link like the browser, sometimes you need to link with a custom lib (like if you are doing NSPR development). Multiple options aren't a big problem IMO if they can be contained reasonably to the build system; multiple options start to be a problem when they have large effects on the code. With this patch, the default is no NSPR at all, but I imagine that will change somehow when the eliminate-non-threadsafe patch lands.
Oh, it's probably also worth noting that --system-nspr doesn't really work on Windows. This technique should. If anybody out there is on Windows and can test this patch, I would totally owe you beer.
(In reply to comment #3) > What should be default? Depends if your goal is a fast initial build or > ensuring correctness/browser-behaviour. Good point. I guess we should go embedded by default. That simplifies lots of things too, and probably makes it a lot easier to land bug 588424. > With this patch, the default is no NSPR at all, but I imagine that will change > somehow when the eliminate-non-threadsafe patch lands. IMO this patch should make embedded the default. That has the advantage of not needing the flags for embedded, which simplifies the interface for users.
Attachment #503971 - Attachment is obsolete: true
Attachment #504132 - Flags: review?(jimb)
Attachment #503971 - Flags: review?(jimb)
This patch takes Paul's suggestion to make an embedded NSPR the default if - we haven't been told NSPR_LIBS or NSPR_CFLAGS (e.g. via browser build) - we haven't been told use --system-NSPR I took the liberty of roughly incorporating the "remove --enable-threadsafe" stuff (3-5 LOC?) from bug 588424 since these options really need to work together and trying to apply both sets of patches as they stand would result in an ugly merge. To be clear, if this patch is r+ then this patch would supercede the patch in bug 588424. I hope this is okay. Would it be better to split the functionality out, submit a new patch for 588424 and then make the bugs interdependent with a note that they have to land with the same push?
Attachment #504132 - Attachment is obsolete: true
Attachment #504142 - Flags: review?(jimb)
Attachment #504132 - Flags: review?(jimb)
Attachment #504142 - Attachment is patch: true
Attachment #504142 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #7) > To be clear, if this patch is r+ then this patch would supercede the patch in > bug 588424. I hope this is okay. Sounds great.
Blocks: 588424
Blocks: 638208
So, the overall goal here is to allow people to build SpiderMonkey without building NSPR first --- right?
I don't want to be discouraging, but I have to say, I am not a fan of this overall approach. It seems fragile and surprising. I guess I don't see why it's critical to avoid requiring an NSPR install. Dependencies aren't pleasant, but lots of packages have them, so it's something people are familiar with, and have tools to help them manage. This is bespoke machinery that only benefits people building out of a Mozilla tree.
(In reply to comment #4) > Oh, it's probably also worth noting that --system-nspr doesn't really work on > Windows. This technique should. If anybody out there is on Windows and can test > this patch, I would totally owe you beer. jorendorff does all his Mozilla work on Windows. (He was concerned that we didn't have enough developers on what is, market-wise, our primary platform, and decided to take one for the team. If I tried this, my moods might affect my family life.)
pbiggar, comments? This is mostly a "why are we doing this" / "what is the goal" question than anything else.
Comment on attachment 504142 [details] [diff] [review] Patch to remove --enable-threadsafe and automatically embed NSPR unless other options are given "Ugly, reliable, and makes everyone happy" is a close friend of mine, and will remain so. I'm just worried that this doesn't solve a serious problem. Requiring an NSPR build doesn't seem like a serious problem.
Attachment #504142 - Flags: review?(jimb) → review-
(In reply to comment #13) > "Ugly, reliable, and makes everyone happy" is a close friend of mine, and will > remain so. I'm just worried that this doesn't solve a serious problem. > Requiring an NSPR build doesn't seem like a serious problem. Really, the problem is that NSPR is nasty on Windows. I wrote docs for it, but its hard and nasty. dmandelin held up the JS_THREADSAFE change because of this, and he was probably right. Perhaps the implementation isn't right, but the idea of using the nsprpub directory is ideal. There is a small benefit of using the same version of NSPR that firefox uses. Finally, cross-compiling is pretty unpleasant with NSPR. You can't just use the system NSPR, and I can't figure out how to make it autodetect, so you need --with-nspr-cflags etc. This fixes it.
(In reply to comment #14) > Really, the problem is that NSPR is nasty on Windows. I wrote docs for it, but > its hard and nasty. dmandelin held up the JS_THREADSAFE change because of this, > and he was probably right. It's not just 'configure; make; make install'? I should read these docs; what's the link?
> (In reply to comment #15) > It's not just 'configure; make; make install'? I should read these docs; what's > the link? Note that I'm talking about the NSPR-spidermonkey interaction, I think NSPR is just |configure,make,make install| (unless we're cross compiling of course ;)) Docs: https://developer.mozilla.org/En/SpiderMonkey/Build_Documentation#NSPR_on_Windows This patch even removes the need to add PATH.
Those docs say: "On Windows, nspr-config returns incorrect values, and so the compiler and linker flags must be given directly." Isn't this the bug? If it's not practical to make nspr-config actually do its job... well, maybe we have people bugs to fix. I keep seeing people tremble when changes to NSPR are discussed; that is, frankly, a problem.
I filed that bug, and then closed it, because nspr-config is for unix-like systems, and we use Visual C++ on windows. NSPR is a pain in the arse, for us and for our users. But we need it, so we should make the process as pain-free as possible.
(In reply to comment #18) > NSPR is a pain in the arse, for us and for our users. But we need it, so we > should make the process as pain-free as possible. Certainly, but that doesn't mean minimizing keystrokes. Windows developers must have some established practice for building packages that have dependencies on other packages. > I filed that bug, and then closed it, because nspr-config is for unix-like > systems, and we use Visual C++ on windows. Don't we still use configure and GNU make on Windows?
(In reply to comment #19) > (In reply to comment #18) > > NSPR is a pain in the arse, for us and for our users. But we need it, so we > > should make the process as pain-free as possible. > > Certainly, but that doesn't mean minimizing keystrokes. I'm not sure what you mean. With this change, they don't need to do anything to get NSPR, without it they do. > Windows developers must > have some established practice for building packages that have dependencies on > other packages. We use our unix-y build process on Windows. They need to install NSPR the way that's described in the docs, AIUI. (Our other dependencies, like hg and pymake, are installed using the MozillaBuild package). > > I filed that bug, and then closed it, because nspr-config is for unix-like > > systems, and we use Visual C++ on windows. > > Don't we still use configure and GNU make on Windows? Yes, but we use Vistual C++ (the compiler, not the IDE) which has different flags and options to Unix compilers.
(In reply to comment #20) > > Don't we still use configure and GNU make on Windows? > > Yes, but we use Visual C++ (the compiler, not the IDE) which has different > flags and options to Unix compilers. Right; can the nspr-config script created as the result of a Windows build return the analogous VC++ flags? It seems like AM_PATH_NSPR in js/src/build/autoconf/nspr.m4 will do the right thing with it, even if the output from nspr-config --cflags and nspr-config --libs produces non-GCC flags.
We can't make nspr-config produce VC++ flags at all it seems. See bug 636615. I think you're focussing on the wrong point, which is whether or not we can make nspr-config do something useful. The patch makes the nspr-dependency a non-issue (esp for cross compiles, where we had a really unpleasant solution anyway), and so does a lot more than fix the nasty nspr-windows issue. Even if the windows thing wasn't an issue, I would still want this patch.
(In reply to comment #22) > We can't make nspr-config produce VC++ flags at all it seems. See bug 636615. That bug doesn't seem to say much more than has been said here. It says that the flags generated *are* wrong, without explaining why they can't be fixed. When other packages use NSPR on Windows (do any?), how do they do it? Is incorporating the thing into one's source tree the only way, or can it be installed and used as an external package? If the latter, how do its clients find the appropriate compiler and linker flags? If there is no established arrangement... why not make nspr-config work there? I feel sure that I'm missing something, but the sticking point seems to be that "One doesn't use nspr-config on Windows." My question is, Why doesn't Windows have the same problem that motivated the invention of *-config scripts on Linux? If it does have that problem, why won't the same solution serve? > I think you're focussing on the wrong point, which is whether or not we can > make nspr-config do something useful. The patch makes the nspr-dependency a > non-issue (esp for cross compiles, where we had a really unpleasant solution > anyway), and so does a lot more than fix the nasty nspr-windows issue. Even if > the windows thing wasn't an issue, I would still want this patch. I'm focussing on this point only because it seems like the most promising alternative to the patch in this bug, which I think is too unlike everything else we do, and will be yet another thing people need to know about. I don't think requiring folks to have an NSPR install somewhere in their paths is much of a burden; it is a burden on Windows because our machinery is broken there, so we should fix the machinery, not invent entirely new stuff that is unfamiliar and won't work for anyone else.
(In reply to comment #23) > (In reply to comment #22) > > We can't make nspr-config produce VC++ flags at all it seems. See bug 636615. > > That bug doesn't seem to say much more than has been said here. It says that > the flags generated *are* wrong, without explaining why they can't be fixed. nspr-config assumes it's generating flags for a unix-like compiler, which uses -l and -L. It has no way of knowing that you're looking to use VisualC++. Also, SpiderMonkey would be the only consumer of such an option, if it existed. (In general, Windows projects use Visual Studio project files, and Cygwin would use the standard flags anywasy). I think we could probably add a bit of configury to translate the options appropriately on windows. Nasty, but not compared to this patch. > When other packages use NSPR on Windows (do any?), how do they do it? Is > incorporating the thing into one's source tree the only way, or can it be > installed and used as an external package? Windows programs typically install all the non-SDK libraries they need. > If the latter, how do its clients > find the appropriate compiler and linker flags? If there is no established > arrangement... why not make nspr-config work there? I don't think there is. I don't see a good reason to invent one though. We can't make nspr-config work because we don't know what compiler the user wants. > > I think you're focussing on the wrong point, which is whether or not we can > > make nspr-config do something useful. The patch makes the nspr-dependency a > > non-issue (esp for cross compiles, where we had a really unpleasant solution > > anyway), and so does a lot more than fix the nasty nspr-windows issue. Even if > > the windows thing wasn't an issue, I would still want this patch. > > I'm focussing on this point only because it seems like the most promising > alternative to the patch in this bug, which I think is too unlike everything > else we do, and will be yet another thing people need to know about. I think the opposite is true, no-one will need to know about it at all. No-one needs to know anything about ctypes, nanojit, nitro, etc, they all just work. > I don't think requiring folks to have an NSPR install somewhere in their paths > is much of a burden; it is a burden on Windows because our machinery is broken > there, We can't fix the machinery. Also, the burden is great for cross-compilation on all platforms. Everyone needs source installs there. > so we should fix the machinery, not invent entirely new stuff that is > unfamiliar and won't work for anyone else. I'm not sure what you mean here. What are we inventing? I don't know what you mean by "won't work for anyone else'? Do you mean other consumers of NSPR? If so, that's a non-issue; this issue only exists because we are weird.
(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > We can't make nspr-config produce VC++ flags at all it seems. See bug 636615. > > > > That bug doesn't seem to say much more than has been said here. It says that > > the flags generated *are* wrong, without explaining why they can't be fixed. > > nspr-config assumes it's generating flags for a unix-like compiler, which uses > -l and -L. It has no way of knowing that you're looking to use VisualC++. What I'm suggesting is that, when NSPR is built, the generated nspr-config script should print flags appropriate for the compiler NSPR was built with. Thus, the only assumption we'd be making would be that NSPR and SpiderMonkey were built with the same compiler. > I think we could probably add a bit of configury to translate the options > appropriately on windows. Nasty, but not compared to this patch. That's something I'd be happier with than the current patch. > No-one > needs to know anything about ctypes, nanojit, nitro, etc, they all just work. Those have all been copied into the SM source tree, so it makes sense that they should be integrated into the SM build system. If SM included its own copy of NSPR, then it would be fine to build that as part of SM, just like ctypes, nanojit, etc. because that's what people have come to expect of such arrangements. What I dislike about this patch is that it reaches outside its tree and builds its own copy of NSPR. There would be two places in the tree that build NSPR (although only one is used at a time).
Chatting on IRC, I think there is a bit of a disagreement as to approaches. jimb (and I believe ted) feel that reaching out of the js/src/ directory to find nsprpub is unclean, but aren't opposed to the goals. I feel that the uncleanness is outweighed by removing the burden of getting NSPR. Not really sure where we can go from here, it looks like a bit of an impasse.
I lean toward Jim and Ted's weighting, but if there really is a Windows build config bug to fix, instead of "fork around", that seems even more clear of a "right thing to do" resolution. Why not try? /be
(In reply to comment #27) > if there really is a Windows build > config bug to fix, instead of "fork around", that seems even more clear of a > "right thing to do" resolution. Why not try? Windows is just one issue that this tries to solve. The others are: - bad usability of nspr configure options, especially on windows - cross-compilation requires source installation of NSPR, and adding the cross-compiled nspr-config to the PATH ahead of the native version of nspr-config. - we need a special PATH on windows, which is unobvious. The reason to not just do the build config is that it doesn't make NSPR invisible to users, which is what this patch is designed to do.
Oooh, I may have a middle-ground. How about we copy nspr and jemalloc into js/src, in the same way we did for js/src/config and js/src/build? Then we can add a sync rule to make to ensure they're identical. The reason this might be a good middle-ground is that we no longer reach out to below the js/src directory. Thoughts?
Paul, thanks for trying to find a better solution. I'm not sure what is best, but copying all of NSPR makes me flinch, because we use so little. Long ago (1998 or thereabouts) we forked NSPR into SpiderMonkey to make it stand alone. This was for !JS_THREADSAFE, though. So could we not fork just the thread stuff? Even that is "big". We use locks, condvars, and a few odds and ends, which has led some embedders to make a "mininspr" with just those interfaces implemented for platforms that they care about. I'm not looking to make work for anyone, but I thought I would throw out this precedent. There was a move by cjones et al. to make a C++0x-compatible threads veneer for js/src and the rest of modern Mozilla code to use. Not sure where that left off either. If we're doing to do something short-term it can be ugly, I guess. I'm looking for a win-win longer term. /be
(In reply to comment #30) > There was a move by cjones et al. to make a C++0x-compatible threads veneer for > js/src and the rest of modern Mozilla code to use. Not sure where that left off > either. Bug 507718. It's in pretty good shape for POSIX platforms, plan on getting that in post-4.0. Still needs a windows >= vista impl, but that code will need to fall back on NSPR everywhere non-POSIX and windows < vista. Not sure how many problems that might solve here (tl;dr).
(In reply to comment #30) > Paul, thanks for trying to find a better solution. I'm not sure what is best, > but copying all of NSPR makes me flinch, because we use so little. Long ago > (1998 or thereabouts) we forked NSPR into SpiderMonkey to make it stand alone. > This was for !JS_THREADSAFE, though. So could we not fork just the thread > stuff? Even that is "big". I'm not proposing copying all of NSPR into JS. Rather, I'm proposing altering our build system to simplify our usage of NSPR, using the mechanism of copying the NSPR directory. Now, in practice, these two amount to the same thing, but they are conceptually different. Even if we only use a small portion of NSPR, we still link to the whole thing. Reducing the amount of NSPR that we use is worthwhile, but that's not this bug. > There was a move by cjones et al. to make a C++0x-compatible threads veneer for > js/src and the rest of modern Mozilla code to use. Not sure where that left off > either. That still relies on NSPR as a fallback on many platforms. There's also a #include of some NSPR stuff in ctypes, but I don't know the extent of it. > If we're doing to do something short-term it can be ugly, I guess. I'm looking > for a win-win longer term. So you're OK with a solution that involves copying nsprpub (and later jemalloc) into js/src? If we do manage to remove the dependency, or change circumstances so that this isn't important, then we can always delete it.
Here's my take, for what it's worth. As I understand it, what Paul wants to achieve here is a simple 'configure; make' build process for threadsafe SpiderMonkey on all platforms, even when one doesn't have NSPR installed yet. My comments above explain why I'm not enthusiastic about the patch, but more fundamentally, I don't think it's as important a goal as Paul does. I think it's fine to require people to build NSPR before configuring SpiderMonkey --- if all the parts work. Unfortunately, not all the parts work. That is what I think we should fix instead. Specifically: - Installing NSPR should create an nspr-config script that accurately conveys the flags needed to compile and link against it, on all platforms and when cross-compiling. (bug 636615) - Mozilla's and SpiderMonkey's configure scripts should take a '--with-nspr' option whose value is the path to the 'nspr-config' script to run. If the value is omitted, configure should try to run nspr-config from the search path. This allows one to have several NSPR installs, doesn't require one to tweak one's PATH, and still makes it easy to use a system-provided NSPR. (We should continue to support --with-nspr-cflags and --with-nspr-libs for emergencies, but no ordinary build process should require them.) With those changes, building SpiderMonkey out-of-tree with no installed NSPR will require the following steps: (cd $NSPR_SOURCE; configure --prefix=$NSPR_INSTALL && make && make install) (cd $SM_SOURCE; configure --with-nspr=$NSPR_INSTALL/bin/nspr-config && make) Naturally, one can build NSPR once and then have multiple SpiderMonkey builds use that installation.
(In reply to comment #33) > (cd $NSPR_SOURCE; configure --prefix=$NSPR_INSTALL && make && make install) > (cd $SM_SOURCE; configure --with-nspr=$NSPR_INSTALL/bin/nspr-config && make) I don't wish to restart the discussion, except to mention that the above needs to be tweaked for cross-compiling, and windows users still need to add the NSPR libs to their PATH before calling js.exe. The latter is probably solvable.
Blocks: 679825
I confess that I haven't really read the whole dicussion above, but this bug popped up in a search and I believe I implemented this separately a while back. It's called --enable-nspr-build, and is the default on Windows. The JS engine now uses mfbt and mozglue (?) and things, so reaching out of js/src doesn't especially disturb me.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: