Closed Bug 244770 Opened 20 years ago Closed 19 years ago

Roaming access not included in installer builds

Categories

(SeaMonkey :: Build Config, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sluggo, Assigned: BenB)

References

Details

(Keywords: fixed1.8)

Attachments

(4 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040526
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040526

The sroaming stuff checked in in bug 124029 still doesn't seem to be in the
daily Windows builds. Can this be enabled?

Reproducible: Always
Steps to Reproduce:
Roaming access has been included in the daily tarball/zip builds (noted by the
presence of sroaming.jar & sroaming.dll).  However, the installer manifests were
not updated to include them.
Assignee: nobody → ben.bucksch
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Summary: Roaming access not included in daily builds → Roaming access not included in installer builds
This will add the required library into packaged binrary. But I'm not sure
about the installer
Attached patch Patch, v2 (obsolete) — Splinter Review
> Index: packages-win
> +; For Raoming Access
> +bin/components/libsroaming.so

I don't think that'll work ;-)

I am attaching a new patch, should fix all platforms and static builds.

I included xpt files, although they are not needed or built by the current
code, just to be safe later.

I didn't add a new component (which would allow users to not install the
roaming component), it didn't seem worth it.

Didn't test yet, will do later.
Attachment #149733 - Attachment is obsolete: true
Attachment #149756 - Flags: review?(cls)
Comment on attachment 149756 [details] [diff] [review]
Patch, v2

Let's not include non-existant files 'just in case' as they will generate
warnings that will confuse people.  At a glance, it doesn't look like sroaming
is being forced to be a dll in a static build so it doesn't need to be listed
in the static build manifest.  (It's also not setting EXPORT_LIBRARY=1 to make
it be included in the final static link list so it's not being included in
static builds at all.)	I wouldn't bother with the package-mac changes.  Afaik,
that file is no longer used.
Attachment #149756 - Flags: review?(cls) → review-
Comment on attachment 149756 [details] [diff] [review]
Patch, v2

> At a glance, it doesn't look like sroaming
> is being forced to be a dll in a static build so it doesn't need to be listed
> in the static build manifest.  (It's also not setting EXPORT_LIBRARY=1 to make
> it be included in the final static link list so it's not being included in
> static builds at all.)

So, what should I do for them, in make and packages file? I don't build static.
(In reply to comment #5)
> So, what should I do for them, in make and packages file? I don't build static.

Add EXPORT_LIBRARY=1 to sroaming/src/Makefile.in and just add sroaming.jar to
the static manifests.


Attachment #149756 - Attachment is obsolete: true
Attachment #149784 - Flags: review?(cls)
Attachment #149784 - Flags: review?(cls) → review+
Checked in yesterday. Marking FIXED.

Filed bug 245721 about static builds.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
People say this is still not working
<http://forums.mozillazine.org/viewtopic.php?p=567864#567864>.

I probably need to add these *dreaded* registerChrome()s to browser.jst as well.
I hate it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Is this correct? Completely untested.
Priority: -- → P2
Comment on attachment 150332 [details] [diff] [review]
Proposed Fix for chrome reg, v1

Yes, works for me, unless I overlooked something.

I'm still not sure it's the right fix, because there's no precedent for an
extension bundled with he browser, which has there skin and locale in its own
jar file. Looks odd, but right to me, though.
Attachment #150332 - Flags: review?(cls)
Attachment #150332 - Attachment description: Fix? for chrome reg, v1 → Proposed Fix for chrome reg, v1
Severity: enhancement → normal
Priority: P2 → --
Comment on attachment 150332 [details] [diff] [review]
Proposed Fix for chrome reg, v1

I'm not the right person to review this.
Attachment #150332 - Flags: review?(cls)
Attachment #150332 - Flags: review?(dveditz)
Attachment #150332 - Flags: superreview?(dveditz)
Attachment #150332 - Flags: review?(dveditz)
Attachment #150332 - Flags: review?(bsmedberg)
Attachment #149784 - Attachment description: Patch, v3 → Patch, v3 -- checked in
Still no roaming in installer builds.
Flags: blocking1.8a2?
Flags: blocking1.8a2? → blocking1.8a2-
Asa, please re-triage this bug for 1.8a2. It is a minor bug, as I think. The
functionality is in zipped builds, but is not in installer ones.

Fixing this will provide testing of this great feature in beta stage from many
tresters.
Flags: blocking1.8a2- → blocking1.8a2?
Flags: blocking1.8a2? → blocking1.8a2-
Since we are actually bundling this with the browser, we need a better
localization story. cc'ing some appropriate people.
bsmedberg, this was always bundled with the browser (and passed reviews as-is),
it's just not included in the Windows installer by accident, due to this bug.
Separating out the localised part might be a valid concern, but would be another
bug (a bug shared with other extensions, IIRC). So, could you please review this
so that we can finally get rid of this bug?
1.8a3 release: roaming still missing in windows builds, both, net installer AND
full install. (btw. linux gtk-problem still there, but that's another bug.)
Ben: What are we missing here? I really like to see this getting into the 1.8
release.
A review
Flags: blocking1.8a4?
Attachment #150332 - Flags: review?(bsmedberg) → review?(neil.parkwaycc.co.uk)
Comment on attachment 150332 [details] [diff] [review]
Proposed Fix for chrome reg, v1

Dan and Neil: Could we get an review here? It's super simple.
Comment on attachment 150332 [details] [diff] [review]
Proposed Fix for chrome reg, v1

OK, I've figured out what's wrong with this patch. We shouldn't be registering
this in browser.jst. Instead, we should be creating a new .jst file and allow
optional installation. Look up what DOMI does.

Basically, registering any locale from browser.jst is wrong.
Attachment #150332 - Flags: superreview?(dveditz)
Attachment #150332 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #150332 - Flags: review-
Anybody here who could provide the patch we need?
Flags: blocking1.8a4?
Flags: blocking1.8a4-
Flags: blocking1.8a2-
I see the Roaming User prefs on Mac OS X (2004092205-trunk, no installer) and
had seen it y'day on Linux (fedora 2) using an installer (iirc, not in front of
that machine atm) build (2004092107-trunk). is this issue limited to Windows
installer builds?
We cannot ship this one one platform and not the others. I recommend pulling it
out of the builds where it currently is and shooting for the next release. That
this hasn't had any testing in our installer builds (our primary downloads)
makes me very uncomfortable shipping it. 
Flags: blocking1.8a4- → blocking1.8a4+
Flags: blocking1.8a4+
Now we have more 1.8aX to go. The sooner it is turned on the more testing there
will be.
Product: Browser → Seamonkey
Given that it's Jan 05 now and this problem is still active, should a comment be
added to the release notes saying something like the following:

The Mozilla "Roaming User" preference is only available on MS-Windows when
downloading the .zip file installation of Mozilla (ie. not when installing via
the Installer programs)
nominating for 1.8beta.
Flags: blocking1.8b?
not going to block on this but I'm sure we'd consider a reviewed patch.
Flags: blocking1.8b? → blocking1.8b-
Mozilla 1.8b win installer includes roaming files sroaming.jar and sroaming.dll.
Attached patch New approach. (obsolete) — Splinter Review
Not sure this is enough to add sroaming to win builds...
If someone has a win installer build, please test this and report back. Thanks!
+Description Long=SRoaming

not very descriptive...:)
Comment on attachment 183819 [details] [diff] [review]
New approach.

No, it's not. You need to create a jst as well, at least. Please try patches
before attaching them. Thanks anyways.
Attachment #183819 - Attachment is obsolete: true
(In reply to comment #32)
> (From update of attachment 183819 [details] [diff] [review] [edit])
> No, it's not. You need to create a jst as well, at least. Please try patches
> before attaching them. Thanks anyways.

Actually I did, but I probably chocked on cvs diff flags. I don't have a win
build system or I would have tested it before. I looked at how the thing was
implemented  for DOM Inspector (as suggested in comment 21) and tried to adapt
it for this.
Do you want me to attach the jst file anyway?
Attached file sroaming.jst
This is the missing sroaming.jst file from Giacomo.
Attached patch sroaming windows install patch (obsolete) — Splinter Review
This is Giacomo's patch from before, with the additional section that moves the
roaming parts into their own section in packages-win. I couldn't get
sroaming.jst into this patch, so it's attached as a separate new file. I have
tested this and built an installer that successfully installs sroaming. There's
possibly scope for some strings fixing but otherwise it's fine.
Attachment #187601 - Flags: review?
Attachment #187600 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 187601 [details] [diff] [review]
sroaming windows install patch

Ben, can you review this?

Thanks,

Prog.
Attachment #187601 - Flags: review? → review?(ben.bucksch)
Comment on attachment 187601 [details] [diff] [review]
sroaming windows install patch

+C13=Component RPT
 ; Make sure Component QFA is LAST so 3rd party developers who might not want
 ; this component can easily remove it.
-C11=Component DOM Inspector
 C12=Component QFA
-C13=Component RPT

QFA should be C14

r=BenB, if that's fixed.

I don't know where locale is supposed to be.
Attachment #187601 - Flags: review?(ben.bucksch) → review-
Comment on attachment 187600 [details]
sroaming.jst

And thanks for taking proper action here!
Attachment #187600 - Flags: review+
Fixed nit (I also fixed the description as noted in comment 31). Requesting r
and sr (parkwaycc at suggestion of #seamonkey)
Attachment #187601 - Attachment is obsolete: true
Attachment #187678 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #187678 - Flags: review?(ben.bucksch)
Comment on attachment 187678 [details] [diff] [review]
config files patch, v3

It wasn't a nit, I had breakage because of misnumbering.

Thanks, James and Giacomo
Attachment #187678 - Attachment description: sroaming with nit fixed → config files patch, v3
Attachment #187678 - Flags: review?(ben.bucksch) → review+
Comment on attachment 187678 [details] [diff] [review]
config files patch, v3

sr=me applies to the .jst file too, of course.
Attachment #187678 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Nominating for seamonkey1.0a - this is a 4xp feature that seamonkey users will
find useful and can be used in marketing.
Flags: blocking-seamonkey1.0a?
(In reply to comment #42)
> Nominating for seamonkey1.0a - this is a 4xp feature that seamonkey users will
> find useful and can be used in marketing.

Shouldn't you just get approval 1.8b4 if you want this for the 1.8 branch?

(In reply to comment #43)
> Shouldn't you just get approval 1.8b4 if you want this for the 1.8 branch?

Is SeaMonkey 1.0 going to be released off the Gecko 1.8 branch?
Yes, SeaMonkey 1.0 will be released off the 1.8 branch. Please set approval1.8b4
then (I'll let you do this so you get bugmail when approval gets granted :).
Hmm, I can't see approval1.8b4 in the list of flags any more.
Comment on attachment 187678 [details] [diff] [review]
config files patch, v3

Oh, approval1.8b4 is under edit patch, not on the main page.
Attachment #187678 - Flags: approval1.8b4?
Features usually don't block releases, and not even our first Alpha. This
doesn't mean it can't go in on the branch - on the contrary, if it dgot
approval, we're happy to get it into branch as well.
Flags: blocking-seamonkey1.0a? → blocking-seamonkey1.0a-
Checking in xpinstall/packager/packages-win;
/cvsroot/mozilla/xpinstall/packager/packages-win,v  <--  packages-win
new revision: 1.294; previous revision: 1.293
done
Checking in xpinstall/packager/windows/config.it;
/cvsroot/mozilla/xpinstall/packager/windows/config.it,v  <--  config.it
new revision: 1.147; previous revision: 1.146
done
Checking in xpinstall/packager/windows/makeall.pl;
/cvsroot/mozilla/xpinstall/packager/windows/makeall.pl,v  <--  makeall.pl
new revision: 1.76; previous revision: 1.75
done
RCS file: /cvsroot/mozilla/xpinstall/packager/windows/sroaming.jst,v
done
Checking in xpinstall/packager/windows/sroaming.jst;
/cvsroot/mozilla/xpinstall/packager/windows/sroaming.jst,v  <--  sroaming.jst
initial revision: 1.1
done
Attachment #187678 - Flags: approval1.8b4? → approval1.8b4+
MOZILLA_1_8_BRANCH: (I assume this is fixed for the branch; if not, please
remove the fixed1.8 keyword)

Checking in xpinstall/packager/packages-win;
/cvsroot/mozilla/xpinstall/packager/packages-win,v  <--  packages-win
new revision: 1.291.4.5; previous revision: 1.291.4.4
done
Checking in xpinstall/packager/windows/config.it;
/cvsroot/mozilla/xpinstall/packager/windows/config.it,v  <--  config.it
new revision: 1.146.4.1; previous revision: 1.146
done
Checking in xpinstall/packager/windows/makeall.pl;
/cvsroot/mozilla/xpinstall/packager/windows/makeall.pl,v  <--  makeall.pl
new revision: 1.75.4.1; previous revision: 1.75
done
Checking in xpinstall/packager/windows/sroaming.jst;
/cvsroot/mozilla/xpinstall/packager/windows/sroaming.jst,v  <--  sroaming.jst
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Keywords: fixed1.8
Thanks, biesi!

I assume this fixes it, marking so. Somebody please verify.
Status: REOPENED → RESOLVED
Closed: 20 years ago19 years ago
Resolution: --- → FIXED
Well, I downloaded a build from
ftp://ftp.mozilla.org/pub/mozilla.org/seamonkey/tinderbox-builds/CREATURE and
while roaming is included, the settings don't save. Although the same happens
with the zip from the same place, so it's a different bug.
You need to log in before you can comment on or make changes to this bug.