Bugzilla Bug 166715

Version 4.9c of xtrs has been out for a while now, and it includes several new
features, including one that I wrote (and gave to Tim Mann) that enhances
security (it optionally prevents the TRS-80 from accessing the Linux
filesystem).  In addition, attempting to emulate actual TRS-80 speed was not
functioning before due to compiler optimization, and this works now.

This is my first ebuild submission, so three questions:

1) Two of the URLs in the old SRC_URI were no longer valid (the original
submitter's site, I presume).  I have changed the main source location to the
xtrs author's site (which seems more appropriate anyway), but one stale one
remains.  Not sure what to do about that.

2) I have not included previous ebuild versions in the attachment.  I assume
this is correct.

3) I put an entry in ChangeLog with my name, but I would guess a developer will
be the one to make this entry for real.  Also, please check formatting of this
entry.

I have tested this in my portage overlay, and it works.  Let me know if there
are any comments or questions.  I have enjoyed contributing to Tim Mann's xtrs
in the past, and I look forward to seeing this new version in Gentoo!  I'd also
be interested in maintaining this package in the future, assuming I become a
developer at some point.


Reproducible: Always

Created an attachment (id=110096) [edit]
New xtrs 4.9c ebuild and associated files 

Does not include new xtrs source code - this can be fetched from the SRC_URI.

Please attach the individual files as plain text.

Created an attachment (id=110117) [edit]
xtrs-4.9c.ebuild

Created an attachment (id=110118) [edit]
ChangeLog

Created an attachment (id=110120) [edit]
Manifest

Created an attachment (id=110121) [edit]
xtrs-4.9c-gentoo.patch

(From update of attachment 110120 [edit])
Removed, since it's generated by ebuild

Created an attachment (id=110133) [edit]
ChangeLog

Created an attachment (id=110134) [edit]
xtrs-4.9c.ebuild

Corrected to install included roms and disk images from xtrs src rather than
from other LDOS dist file (which contains non-up-to-date versions).  Also made
DESCRIPTION more correct and brief.

The new ebuild works, except there is one issue left-over from the original:

There are two dist files:

* xtrs-4.9.tar.gz
* ld4-631.tar.gz

The former is the source, which includes a couple of ROMs and a couple of disk
images (but no LDOS disks).  This part is fine.  The latter contains some
redundant ROMs & disks (some are old versions now), two symlinks, and the LDOS
disk image.  The LDOS disk (and symlinks) are not contained in the source, but
the rest is.  The installed disks and ROMs (except for LDOS) should come from
the new xtrs source, not ld4-631.tar.gz.

So I have updated the ebuild to only extract the symlinks and LDOS disk from
ld4-631.tar.gz and get the rest from the new xtrs source.  This makes
ld4-631.tar.gz contain some redundant/unused info.  Also, this is the file for
which the SRC_URI is no longer valid.  Basically, even though the ebuild works
and installs the proper files, this issue should probably be cleaned up.

My question is how best to do this.  Either ld4-631.tar.gz could be updated to
contain only the LDOS disk and the symlinks could be made with ebuild scripts,
or the LDOS disk image could be placed elsewhere.  I am not sure how we'd
update ld4-631.tar.gz itself, since it's cached in Gentoo.  Would it be right
to make a new valid SRC_URI entry for it?  It seems this could cause
unpredictability on which "ld4-631.tar.gz" is fetched...

If nobody else steps up, I can take this one.

Ulrich,

I've been in touch with Tim Mann, and he's looking into another patch I wrote
right now, so OK to hold off, since this bug has been dormant for so long.  We
can safely wait until the next xtrs version, I think (or he he wants me to
include my patch in this ebuild).  I'll post back here when I hear back, or I
can close this one out, whichever works better.

-Joe

Joe,

both possibilities are fine with me. If the upstream version is to be expected
soon, then we should probably wait for it. Otherwise it is also fine with me to
include your patch in the Gentoo version. (I have contributed some pieces to
xtrs myself and know the code reasonably well.)

Resolving as LATER. Please reopen if there are news.

Created an attachment (id=116937) [edit]
ChangeLog

Created an attachment (id=116938) [edit]
xtrs-4.9c.ebuild

Created an attachment (id=116940) [edit]
files/xtrs-4.9c-gentoo.patch

OK, I bit the bullet and got everything up to date.  See the change log for
what this includes.  I know Tim Mann is pretty busy right now, and I think my
fixes are good patches to 4.9c.  When I hear back from him (esp. if he makes a
whole new release), we may want to bump this again.

Thanks for picking this one up!

Created an attachment (id=116941) [edit]
ChangeLog

Minor tweak: updated date within change entry

OK, great!

Please allow me some hints about the ebuild:

- KEYWORDS should be changed to unstable.
- virtual/x11 is obsolete and should be removed (keep only libX11).
- Don't drop all CFLAGS unconditionally. Use flag-o-matic.eclass
  and remove any unwanted optimisations with filter-flags etc.
- It is now recommended to do "emake install", not "make install".

(And it is not necessary to attach the complete ChangeLog.)

Thanks for the tips!  This was my first ebuild, so I'm learning about new
features, etc.  To not drop CFLAGS, I assume Makefile should be patched not to
override it...  And yes, I suppose filtering out optimization flags would be
the trick here to achieve the effect of allowing real-time run speed.  Also,
yes, makes perfect sense on setting it unstable.  Do you want me to re-submit
the ebuild after making these changes?

(In reply to comment #20)
> Do you want me to re-submit the ebuild after making these changes?

Yes, please.

OK, I've done all except the CFLAGS part - I'll have to look up how to do
that...  I'll repost the patch and ebuild after I get that done.

Created an attachment (id=116955) [edit]
xtrs-4.9c.ebuild

Created an attachment (id=116957) [edit]
files/xtrs-4.9c-gentoo.patch

Created an attachment (id=116959) [edit]
files/xtrs-4.9c-gentoo.patch

Ulrich,

Take a look at the two new files (ebuild and patch).  I believe I have
addressed all of your tips.  Let me know if it still needs work.  Note that I
also made use of append-flags for the endian ppc flag rather than modifying
Makefile.local dynamically - seemed more clean to me.

(In reply to comment #26)
O.K., thanks. I'll probably look into it during next week.

Oops, missed one. ;-) First SRC_URI should be .../${P}.tar.gz. Don't put
version numbers there. See:
<http://www.gentoo.org/proj/en/devrel/handbook/handbook.xml?part=2&chap=3#doc_chap1>.

> First SRC_URI should be .../${P}.tar.gz. Don't put version numbers there.

No need to resubmit the ebuild, though.

Created an attachment (id=116967) [edit]
xtrs-4.9c-delay-volatile.patch

I'm not fully convinced that switching off optimisation globally is the best
solution here.

Could you apply this patch and try if the delay works properly then? With
optimisation?

Ulrich:

On the version in the SRI, thanks for spotting that (not usng ${P}).

On optimization, I tried your patch.  Interestingly, it requires me to use a
delay that is 1/3 as large, meaning each cycle of the delay takes longer with
the volatile int and -O2.  Once, when running it, the delay seemed unevenly
applied, but I am not sure this is due to using the optimizer (certainly, the
pattern of program flow will be slightly different using -O2 and volatile). 
Also, typing felt sluggish at first, but I think that might be because my
initial delay setting was too long.

I am comfortable using optimization/volatile for the testing phase to see if
there are any real issues.  I will attach a new main patch and ebuild (so don't
include your volatile patch).

Created an attachment (id=116993) [edit]
xtrs-4.9c.ebuild

Created an attachment (id=116995) [edit]
files/xtrs-4.9c-gentoo.patch

Created an attachment (id=117023) [edit]
xtrs-4.9c.ebuild

Tested on Gentoo/FreeBSD successfully; added KEYWORD ~x86-fbsd

Rearranged some things in the ebuild:
- Unpack disk images in src_unpack
- CFLAGS and PREFIX explicitely passed to emake

And in the patch:
- patching of PREFIX removed (see above)
- vpath removed (does not make sense: there is nothing in ${D} at build time)
- Cygwin/SIGIO stuff removed, not needed for Gentoo-specific patch
- restored package ChangeLog line, give proper credit to previous contributor

Joe: Could you test this again?

Another question: Should we keep trs_model=5 as default? The Model I was much
more common and is the upstream default.

Created an attachment (id=117213) [edit]
xtrs-4.9c.ebuild

Created an attachment (id=117215) [edit]
files/xtrs-4.9c-gentoo.patch

OK, tested (on ~x86 & ~x86-fsbd) - it works!

Comments:

1) Note that you really only need to unpack "disks/ld4-631.dsk" from
ld4-631.tar.gz (one of the things I had changed is to grab the other disks from
the main source dist), but since this is just in the working dir and will get
removed anyway, it doesn't matter that much...

2) The vpath and PREFIX stuff I had left unchanged from the previous ebuild -
not sure why it was done that way, but I had not wanted to break what was
intended.  Thanks for evaluating it and cleaning it up.

3) Yeah, I was thinking of removing the SIGIO patch, but I did not want to lose
track of it (it allowed the option of unsetting this, but had no effect here). 
I'll see if Tim wants to add it upstream for the future.

4) The ChangeLog comments now will appear at the top, so they are a little out
of order with Tim's comments (4.9b, c, etc.), but perhaps that's proper (i.e.
having all Gentoo entries at the top).

5) We need to keep the new default for model at "5" (really makes it a TRS-80
4p).  This is because there is no public domain model I ROM, so out of the box,
it cannot work after the emerge if we leave it at model "1".

Thanks!  Joe

(In reply to comment #38)
> OK, tested (on ~x86 & ~x86-fsbd) - it works!

Good. (Beyond starting the emulator I haven't done any serious tests myself
yet; 
I have do dig out my old diskimages for that... Something like "Galaxy Invasion
Plus" might be a good test case. ;-)

Please note that I cannot include the ~x86-fbsd flag immediately; this is the
task of the arch teams. We have to file a keywording bug for that later.

> 1) Note that you really only need to unpack "disks/ld4-631.dsk" from
> ld4-631.tar.gz

I know... Usually one should unpack everything with "unpack ${A}", but it would
be clumsy for ld4-631.tar.gz.

> 5) We need to keep the new default for model at "5" (really makes it a
> TRS-80 4p).  This is because there is no public domain model I ROM, [...]

There are images of the LNW-80 ROM at
<http://www.xs4all.nl/~fjkraan/comp/lnw/rom/>. I don't know about their legal
status though.

So I'll leave the default at Model 4P, but add some instructions how to install
other ROM images.

Added to CVS. Thanks again.

FreeBSD keyword request: Bug #176022.

Awesome - thanks much!  And thanks for submitting the ~x86-fbsd request.