Closed Bug 444050 Opened 11 years ago Closed 9 years ago

move update-packaging shell scripts to python

Categories

(Release Engineering :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: rhelmer, Assigned: coop)

References

Details

(Whiteboard: [updates])

Attachments

(3 files)

Update packaging for nightlies and patcher in it's default mode are done by the following scripts in mozilla/tools/update-packaging/:

* make_full_update.sh # creates full MARs
* make_incremental_update.sh # creates incremental update between two MARs
* unwrap_full_update.(pl,sh) # unwraps full MARs
* common.sh 

The first and second are called by Makefile targets "complete-patch" and "partial-patch". The third and fourth contain utilities for the first two.

make_incremental_updates.py (called by patcher for releases when in "fast mode") does almost all of the needed functions provided by these scripts:

* unwraps full MARs
* create incremental updates between two MARs

It is not able to create full MARs but that's pretty trivial to add.

Maybe make_incremental_updates.py should be turned into a more general "update tools" script, which can be called to perform different functions? I'm not sure about the nightly update generator, but the release update generator (patcher) calls the scripts directly, not through the Makefile; these should probably be standardized to use the Makefile.

I think the reason that they do not is because it requires a full tree checkout (see bug 329686) in order to generate the needed Makefiles. This seems fixable.

I know that Tinderbox does use the Makefile to create complete MARs, and it looks like that's what the moz2 buildbot is doing as well. These already have a full tree checkout so they don't care about bug 329686 :)
Schrep wrote a script in python which needs to be cleaned up and committed. I forget which bug that was in, something about it taking a really long time to generate partial MARs for localized builds.
That script was committed in bug 410806.   I'd be happy to assist in modifying make_incremental_updates.py - I had hoped it was much cleaner/easier to understand than the scripts it replaced.   
(In reply to comment #1)
> Schrep wrote a script in python which needs to be cleaned up and committed. I
> forget which bug that was in, something about it taking a really long time to
> generate partial MARs for localized builds.

Yep - it was committed, that's the script I'm talking about - make_incremental_updates.py - we use it for releases even!

Here's the deal, there are a couple different ways that updates are done right now:

* nightly builders generate complete MAR (using the Makefile) and upload a little special-purpose text file to prometheus-vm; prometheus uses a script (which I think is only in the private repo) that determines what the last complete was and makes a partial. This uses the old shell scripts, calling them directly IIRC.

* release builders use patcher (mozilla/tools/patcher), which in it's default mode use the old shell scripts, and the optional "fast mode" uses make_incremental_updates.py (which we use for releases now, afaik). patcher calls the shell scripts or make_incremental_updates.py directly.

One way to bring this together - make_incremental_updates.py could be renamed and extended to support unpacking MARs, creating full and incremental MARs, and be called from the Makefile instead of the old shell scripts. It'd need to support more command line arguments, and renaming it to "update_tools.py" or something like that would probably be more appropriate, or splitting it into a couple scripts (like we have now e.g. make_complete, make_incremental, unwrap_complete).

patcher and the nightly update generator could at least be made to call the Makefile not call scripts directly. Ideally they should not have such a different process, but there's another bug for that.

(In reply to comment #2)
> That script was committed in bug 410806.   I'd be happy to assist in modifying
> make_incremental_updates.py - I had hoped it was much cleaner/easier to
> understand than the scripts it replaced.   

I think that the original bug was 391958. BTW I asked in there, you mentioned unit tests, did those ever get checked in? We should probably be running those somewhere e.g. unit test boxes... 
schrep/rob: any sign of those unittests for the update generation, so we can make sure we dont break anything?
Component: Release Engineering → Release Engineering: Future
Priority: -- → P3
We need to grab those tests from backups on people.m.o (test.zip in my homedir).  Justin any updates there?
aravind is looking for test.zip on p.m.o in your dir now around the 1/1/08 timeframe...
I have the tape, it will be taken to the datacenter sometime Monday.  Should be able to restore the file then.
Schrep: File restored to your public_html directory.
Rockin - thanks *so* much aravind!
Ok I've got the tests, I'll bring them up to the current state of make_incremental_updates.py (I've added a few features in separate bugs), and file a different bug to make them part of the normal unit test runs.

I will take this bug on, unless someone else is chomping at the bit for it. Let me know if so :)
Assignee: nobody → robert
Attaching schrep's original tests for make_incremental_updates.py
I think it'd be worth making pyunit tests for this, and hooking it up to run as part of regular unit test runs. 

That way we'd know that mar, mbsdiff and the packaging tools at least work against reference MARs on each checkin.
OK so after all the unit test noise - is the original intent of this bug still useful?

My original plan here was to completely do away with these files in tools/update-packaging:

common.sh
make_full_update.sh
make_incremental_update.sh
unwrap_full_update.sh

The Makefile.in would be adjusted so that calling the "full-update", "complete-patch" and "partial-patch" targets would do the right thing.

RE: comment #0 regarding bug 329686, the issue of this being part of the build or not should be left out of this bug IMHO.
(In reply to comment #15)
> OK so after all the unit test noise - is the original intent of this bug still
> useful?

Absolutely. I have "unifying the our update processes" as a goal for this quarter, and this bug is where I was/am planning to start.

Grabbing the bug from you rhelmer, unless you really want to keep it. ;)
Assignee: robert → nobody
Component: Release Engineering: Future → Release Engineering
Depends on: 502612
Assignee: nobody → ccooper
(In reply to comment #16)
> (In reply to comment #15)
> > OK so after all the unit test noise - is the original intent of this bug still
> > useful?
> 
> Absolutely. I have "unifying the our update processes" as a goal for this
> quarter, and this bug is where I was/am planning to start.
> 
> Grabbing the bug from you rhelmer, unless you really want to keep it. ;)

Fine with me to take the bug, I would like to help out so feel free to break off a piece for me :)
Ok so here's the deal - the test.zip in attachment 356844 [details] is really more of a functional test than a unit test, here is how to use it:

run buildrefmars.sh, which:
1) creates reference partial MARs ref-mac.mar and ref.mar using old make_incremental_update.sh
2) creates from.mar, to.mar, from-mac.mar using old make_full_update.sh

run runtests.sh, which:
1) calls make_incremental_updates.py with testpatchfile.txt to create test-mac.mar and test.mar
2) calls diffmar.sh to compare the ref.mar to test.mar and ref-mac.mar to test-mac.mar

In case you're wondering, there are Mac-specific tests because there's a special prefix on Mac "Contents/MacOS", so the test makes sure this works.

The attached patch is necessary because in bug 410806 I added some logic to make_incremental_updates.py so that it can decode filenames and generate metadata that is needed for AUS, so the complete update filenames must be of the form "product-1.0.lang.platform.complete.mar" and not e.g. "from.mar"
Attachment #388787 - Flags: review?(ccooper)
Note that if the shell scripts go away this test is not going to work anymore.. I'd suggest checking in a known-good reference MAR instead of building it. This test was originally intended to show that there were no differences in using the shell scripts versus the python script, AFAICT.

Still, better to start with something than nothing.. in order to run this it needs to be in tools/update-packaging/ and mar/mbsdiff need to be on the PATH.

I think a better strategy would be to have proper unit tests for mar/mbsdiff and separate pyunit tests for make_incremental_updates.py, which could supersede this approach. None of this needs to block this bug IMHO.
(In reply to comment #19)
> Still, better to start with something than nothing.. in order to run this it
> needs to be in tools/update-packaging/ and mar/mbsdiff need to be on the PATH.

I meant to say tools/update-packaging/test/
Unit tests. Much of the interesting stuff is commented out, either because they touch the filesystem or make direct shell calls.

I suggest refactoring make_incremental_updates.py to make it more testable:

1) methods that take filename should take a filehandle instead, then a StringIO could be handed over instead
2) methods that make direct shell calls should instead build the command and return it. Right now exec_shell_cmd() is called internally and uses os.system(), but it'd be simpler to use subprocess.check_call()

However, there are a couple useful tests in here and don't require mar/mbsdiff and should not touch the filesystem or call any commands, so it's safe to take as-is and hook up to be called by unittest builders IMHO.
Attachment #388808 - Flags: review?(ccooper)
Attachment #388787 - Flags: review?(ccooper) → review+
Attachment #388808 - Flags: review?(ccooper) → review+
Sorry for the review delay, rhelmer. I was on vacation for a while there. Your tests look good after running them locally.

Schrep's original test package hasn't landed yet. Do you want me to land everything together, or do you want to handle landing?
(In reply to comment #22)
> Sorry for the review delay, rhelmer. I was on vacation for a while there. Your
> tests look good after running them locally.
> 
> Schrep's original test package hasn't landed yet. Do you want me to land
> everything together, or do you want to handle landing?

If you could land everything I would appreciate it.. do you want either or both of these tests to be run automatically (e.g. by the unittest boxes)? I haven't addressed any of that.
Attachment #356844 - Flags: checked-in+
Attachment #388787 - Flags: checked-in+
Attachment #388808 - Flags: checked-in+
What are the remaining steps here? 

Scanning through the bug I see suggestions for refactoring make_incremental_updates.py to facilitate testing, and setting up regular runs of the unit tests for the update packaging. I know bhearsum has previously expressed interest in setting up unit tests for other parts of the build system, so this might also fit there.

Should we handle those issues in this bug, or file new bugs?
(In reply to comment #25)
> What are the remaining steps here? 
> 
> Scanning through the bug I see suggestions for refactoring
> make_incremental_updates.py to facilitate testing, and setting up regular runs
> of the unit tests for the update packaging. I know bhearsum has previously
> expressed interest in setting up unit tests for other parts of the build
> system, so this might also fit there.
> 
> Should we handle those issues in this bug, or file new bugs?

I think that since the core of this bug is to refactor make_incremental_updates.py into a library, the unit test changes should just be part of those patches.

The task(s) of setting up unit tests for the build system should probably be in a separate bug.

I started working on this a bit, so far I have 
tools/update-packaging/
  updates/
    mar.py
    patch.py

These contain the Mar and PatchInfo classes, and I am refactoring the stuff in make-incremental-updates.py into functions.

The idea is that you could have equivalents to tools/update-packaging/{unwrap_full_update.sh, make_full_update.sh, make_incremental_update.sh}. I was going to stay uncreative and just make drop-in .py equivalents of these files, and hook the Makefile up to these.

That is basically where I am, so if anyone wants to help or take over some or all of this let me know, otherwise I'll plod along.
Status: ASSIGNED → NEW
Priority: P2 → P3
Throwing this one back into the pool.
Assignee: ccooper → nobody
Priority: P3 → P5
Whiteboard: [updates]
Assignee: nobody → jhford
Assignee: jhford → coop
Priority: P5 → P3
We've got some tests now thanks to this bug, and we're using the python version of the script.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.