Open Bug 371178 Opened 15 years ago Updated 13 years ago

Replace version string in mar urls/filenames

Categories

(AUS Graveyard :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

ASSIGNED
4.x (triaged)

People

(Reporter: mozilla, Assigned: mozilla)

Details

Attachments

(3 files, 1 obsolete file)

MozAUSLib actually checks to see if version is set and replaces it in SubstitutePath, but it only gets passed in when dealing with the default mar name. In order to make patcher2.cfg more generic we needed to be able to define the mar urls and filenames with %version% in them.

Attaching a patch with the fixes that seem to be giving us all the "right stuff", and looking at the surrounding code I think I'm pulling the version number from the right places.
Attachment #255953 - Flags: first-review?(preed)
Comment on attachment 255953 [details] [diff] [review]
Add version args to SubstitutePath calls

This looks safe/sane, but I need to do a test run to be sure (which I'm swamped at the moment).

Quick question: can you give me the context for making this change? That might help a bit.
Comment on attachment 255953 [details] [diff] [review]
Add version args to SubstitutePath calls

This is OK.

One thing to note: in patcher2.pl, at 287, you use $r, which are the release versions to use; but for the rest of the definition of "version", you use the "appv" version, which may not be the same thing.

We've been fuzzy on this definition, but appv should always be a base version (2.0.0.2, 1.5.0.10, etc.), but the release version can be things like 1.5.0.10-google. If you're ok with this inconsistency, I suppose I am, but it'll probably bite someone in the future.
Attachment #255953 - Flags: first-review?(preed) → first-review+
Thanks for your comments. Sorry I missed your first question. These are things we need for Songbird. Basically I was just trying to pull version into the other calls, like it was pulled into the replacement for the DEFAULT_MAR_URL. 

I'll take another look at my use of '$r' and see if I can get to the 'appv' version instead. That was just the kind of thing I wasn't sure of, thanks!

As I'll probably be posting another patch, would you be open to adding 'branch' as another replacement token? We use the branch in the path to the uploaded mar file and it would make the config file that much more generic if we could replace on that token. I can put a patch up with that combined in if you'd like to take a look before commenting.

Cheers!
This patch changes the setting of version from using appv to using the name of the release. I changed some existing cases too, which seemed like the correct thing to do.

I've exposed 'build_id' to substitution, which was an existing field, but not part of the replacement. I also added substitution for 'branch', which allowed us to abstract away another field in the config file. This is a new field entirely and it's added to the release section in the config file.

I'll post the config file I'm working from as a demonstration of how I'm using these changes, and my motivation for them.
Assignee: morgamic → mozilla
Attachment #255953 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #256818 - Flags: first-review?(preed)
Attached file sample config file
Config using the changes outlined above.
I should note that I made changes based on the assumption that the release name would be the name desired to be used in all cases, except for the snippet files. Since 0.2.5-foobar could be different than 0.2.5-barfoo there would be a need for different upgrade paths and location of snippets on the server.

That said, I missed one spot that is used for a path in CreatePartialPatchinfo and CreateCompletePatchinfo:

#old
#my $from_aus_version = $from->{'appv'};
#new
my $from_aus_version = $u_config->{$u}->{'from'};

I can repost another patch if needed.
(In reply to comment #3)

> As I'll probably be posting another patch, would you be open to adding 'branch'
> as another replacement token? We use the branch in the path to the uploaded mar
> file and it would make the config file that much more generic if we could
> replace on that token. I can put a patch up with that combined in if you'd like
> to take a look before commenting.

Can we check the patch that I r+'d in, and then post a new patch that includes the new things?

I only ask because I tested the version of the patch that I r+'d, and it looked good, so I want to take these changes in digestible chunks.

Is that ok?
> Is that ok?

Yeah, I've gotten busy on other stuff and have been letting my Moz bugs slip. :-( Smart to do it in smaller chunks anyway.

If you can easily drop those changes in that would be great, but if you're tight on cycles I can get a clean pull and apply the patch to check in. I'll move in that direction and just post here if you get it checked in, thanks.

Haven't forgotten about this. Took me a day to set up a clean patcher directory and figure out which set of mozilla I should pull to get it to build. But have it now and am testing the patch one last time to make sure it does everything correctly.

Should be able to verify it's working and check it in today; and hopefully have a cleaned-up second patch that has the deltas between the two patches currently posted.
Comment on attachment 255953 [details] [diff] [review]
Add version args to SubstitutePath calls

Checked in to trunk.
Attachment #255953 - Attachment is obsolete: false
This is basically the delta between the first two patches. Needed this now that the first patch is checked in.

- adds some comments
- checks to make sure the 'from' release is defined in the config file
- adds parsing of additonal version string from config file for from/to section with fallback
- removes the build_id from the up_config since each platform can have a seperate distinct build_id
- adds parsing/substitution of the 'branch' key/token
- adds build_id and branch to a host of SubstitutePath calls
- changes the version string substituted from 'appv' to what contains the version set as described above (it's named differently in places, but mostly it's 'to_name')
- adds 4 platform mappings to the AUS2_PLATFORMS map

I'll review the attached cfg file, but I don't think that has changed at all, so it should still serve as a reference for what we're trying to get patcher2 to work with.
Attachment #256818 - Attachment is obsolete: true
Attachment #261338 - Flags: first-review?(preed)
Attachment #256818 - Flags: first-review?(preed)
Attachment #261338 - Flags: first-review?(mozpreed) → review?(mozpreed)
Comment on attachment 261338 [details] [diff] [review]
revised patch - version 3 - additions to v1

So, I looked through this and it *looks* ok; I'd like to have the set of tests performed that we did before, i.e. generate a set of snippets, pre- and post-patch, and then look at the diff between the two before I r+ this.

I'm happy to do the test, but I can't do it right now, so that data would speed things along.
adding John and Robert as this is one of the bugs I mentioned in our talk last week. The last patch here is pretty stale and given the fact an attempt is being made to move from patcher2.pl to make_*_updates.py I question whether this work needs to move forward.

If so, I'm happy to post a fresh patch (there's even more substitutions) here.
(In reply to comment #13)
> adding John and Robert as this is one of the bugs I mentioned in our talk last
> week. The last patch here is pretty stale and given the fact an attempt is
> being made to move from patcher2.pl to make_*_updates.py I question whether
> this work needs to move forward.


I think we'll continue to need patcher for a while, so I think it's worth it.

 
> If so, I'm happy to post a fresh patch (there's even more substitutions) here.


That, plus the test preed mentions in comment #12 would be useful, especially to make sure it doesn't break Firefox or Thunderbird.. I (or anyone) could do this now that the patcher configs for Firefox/Thunderbird are public, let me know when you get everything posted.
Comment on attachment 261338 [details] [diff] [review]
revised patch - version 3 - additions to v1

Moving review to morgamic since preed is not responsive.
Attachment #261338 - Flags: review?(mozpreed) → review?(morgamic)
Comment on attachment 261338 [details] [diff] [review]
revised patch - version 3 - additions to v1

I won't drive this any longer and want to get this out of my queue.
Attachment #261338 - Flags: review?(morgamic)
You need to log in before you can comment on or make changes to this bug.