Re: Resolving the same windows test failures again
On 05/05/2015, Andrew Wilkins andrew.wilk...@canonical.com wrote: Those fixes were not made on v5, and only existed on v6-unstable. I later came along and merged some changes into v5 and updated Juju's dependencies.tsv to the latest commit on v5. That meant the fixes were dropped. Not easy to track this history on github, but as far as I can see v6-unstable did not exist when those changes were merged. Looks like v5 was then renamed in the web ui to v6-unstable and v5 was pushed up from an earlier revision. When your branch went to bump the rev, it therefore dropped those changes (and when I went to pull my v5 branch locally, it complained of diverged branches). Martin -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Resolving the same windows test failures again
On Tue, May 5, 2015 at 7:20 AM, Martin Packman martin.pack...@canonical.com wrote: There was some confusion about the regression to the windows test failures on trunk. https://bugs.launchpad.net/juju-core/+bug/1450919 Partly my fault, Curtis initially looked at the 1.24 branch and I looked at trunk, and each branch has a different issue. Here's what I've just done to diagnose. So, trunk first, I noticed the failures looked exactly the same as before CI fiddled with the environment variable casing to work around a juju bug: https://launchpad.net/bugs/1446871 The fix for this bug landed on master, therefore it's easy to assume that change actually broke the casing behaviour in such a way as to invalidate the CI hack, rather than fixing the underlying issue. Looking at the code: https://github.com/juju/juju/pull/2124/files Two similar functions now exist for merging case, mergeEnvWin which works on map[string]string and mergeWindowsEnvironment which works on []string. Only mergeEnvWin has tests. Guess, mergeWindowsEnvironment is bugged. Indeed: - m[strings.ToUpper(varSplit[0])] = varSplit[1] + k := varSplit[0] + if _, ok := uppers[strings.ToUpper(k)]; ok { + uppers[k] = varSplit[1] It's not uppercasing the key in the assignment. So, Path is matched to PATH, Path is assigned to, but later only PATH is pulled out. Now for the 1.24 branch, no hints here. So, lets see what changed. Latest working 1.24 without windows test failures: http://reports.vapour.ws/releases/2581 Earliest 1.24 with windows test failures: http://reports.vapour.ws/releases/2592 $ git diff fffe3e4f..95e7619f 2770 lines... Not helpful. But, error mentions cannot move the charm archive and: -gopkg.in/juju/charm.v5 git 6b74a2771545912f8a91a544b0f28405b99386242015-04-14T14:33:47Z +gopkg.in/juju/charm.v5 git 779394167ac61b02ca73ca17c3012f05a5ba316c2015-04-30T02:46:55Z $ pushd ~/go/src/gopkg.in/juju/charm.v5 Try to update and branches diverged? Wha? $ git diff 6b74a277..77939416 Er... this includes a revert of my licence header changes, and, the answer to the test failures, Gabriel's file closing fix: https://github.com/juju/charm/pull/119 https://github.com/juju/charm/pull/120 Those fixes were not made on v5, and only existed on v6-unstable. I later came along and merged some changes into v5 and updated Juju's dependencies.tsv to the latest commit on v5. That meant the fixes were dropped. I've cherry-picked the fixes into v5, and have a merge job queued for https://github.com/juju/juju/pull/2212. Please, everyone, be careful to ensure fixes go on the appropriate branch. For gopkg.in-based packages, that means only pinning to commits on the branch related to the package path suffix. Cheers, Andrew So, just renaming a variable is *not* safe (if you accidentally back out other changes when merging). Both these regressions happened due to landing 'safe' changes while the branch was broken, and were therefore harder to pin down than they would otherwise have been. Martin -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Resolving the same windows test failures again
Sorry for the lack of tests for that part of the code, I guess I added them for one and not the other. Note that I had actually been leaving that PR unmerged because I knew master was blocked. Sorry it got pushed in anyway, and caused problems. On Mon, May 4, 2015 at 7:20 PM, Martin Packman martin.pack...@canonical.com wrote: There was some confusion about the regression to the windows test failures on trunk. https://bugs.launchpad.net/juju-core/+bug/1450919 Partly my fault, Curtis initially looked at the 1.24 branch and I looked at trunk, and each branch has a different issue. Here's what I've just done to diagnose. So, trunk first, I noticed the failures looked exactly the same as before CI fiddled with the environment variable casing to work around a juju bug: https://launchpad.net/bugs/1446871 The fix for this bug landed on master, therefore it's easy to assume that change actually broke the casing behaviour in such a way as to invalidate the CI hack, rather than fixing the underlying issue. Looking at the code: https://github.com/juju/juju/pull/2124/files Two similar functions now exist for merging case, mergeEnvWin which works on map[string]string and mergeWindowsEnvironment which works on []string. Only mergeEnvWin has tests. Guess, mergeWindowsEnvironment is bugged. Indeed: - m[strings.ToUpper(varSplit[0])] = varSplit[1] + k := varSplit[0] + if _, ok := uppers[strings.ToUpper(k)]; ok { + uppers[k] = varSplit[1] It's not uppercasing the key in the assignment. So, Path is matched to PATH, Path is assigned to, but later only PATH is pulled out. Now for the 1.24 branch, no hints here. So, lets see what changed. Latest working 1.24 without windows test failures: http://reports.vapour.ws/releases/2581 Earliest 1.24 with windows test failures: http://reports.vapour.ws/releases/2592 $ git diff fffe3e4f..95e7619f 2770 lines... Not helpful. But, error mentions cannot move the charm archive and: -gopkg.in/juju/charm.v5 git 6b74a2771545912f8a91a544b0f28405b99386242015-04-14T14:33:47Z +gopkg.in/juju/charm.v5 git 779394167ac61b02ca73ca17c3012f05a5ba316c2015-04-30T02:46:55Z $ pushd ~/go/src/gopkg.in/juju/charm.v5 Try to update and branches diverged? Wha? $ git diff 6b74a277..77939416 Er... this includes a revert of my licence header changes, and, the answer to the test failures, Gabriel's file closing fix: https://github.com/juju/charm/pull/119 https://github.com/juju/charm/pull/120 So, just renaming a variable is *not* safe (if you accidentally back out other changes when merging). Both these regressions happened due to landing 'safe' changes while the branch was broken, and were therefore harder to pin down than they would otherwise have been. Martin -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Resolving the same windows test failures again
There was some confusion about the regression to the windows test failures on trunk. https://bugs.launchpad.net/juju-core/+bug/1450919 Partly my fault, Curtis initially looked at the 1.24 branch and I looked at trunk, and each branch has a different issue. Here's what I've just done to diagnose. So, trunk first, I noticed the failures looked exactly the same as before CI fiddled with the environment variable casing to work around a juju bug: https://launchpad.net/bugs/1446871 The fix for this bug landed on master, therefore it's easy to assume that change actually broke the casing behaviour in such a way as to invalidate the CI hack, rather than fixing the underlying issue. Looking at the code: https://github.com/juju/juju/pull/2124/files Two similar functions now exist for merging case, mergeEnvWin which works on map[string]string and mergeWindowsEnvironment which works on []string. Only mergeEnvWin has tests. Guess, mergeWindowsEnvironment is bugged. Indeed: - m[strings.ToUpper(varSplit[0])] = varSplit[1] + k := varSplit[0] + if _, ok := uppers[strings.ToUpper(k)]; ok { + uppers[k] = varSplit[1] It's not uppercasing the key in the assignment. So, Path is matched to PATH, Path is assigned to, but later only PATH is pulled out. Now for the 1.24 branch, no hints here. So, lets see what changed. Latest working 1.24 without windows test failures: http://reports.vapour.ws/releases/2581 Earliest 1.24 with windows test failures: http://reports.vapour.ws/releases/2592 $ git diff fffe3e4f..95e7619f 2770 lines... Not helpful. But, error mentions cannot move the charm archive and: -gopkg.in/juju/charm.v5 git 6b74a2771545912f8a91a544b0f28405b99386242015-04-14T14:33:47Z +gopkg.in/juju/charm.v5 git 779394167ac61b02ca73ca17c3012f05a5ba316c2015-04-30T02:46:55Z $ pushd ~/go/src/gopkg.in/juju/charm.v5 Try to update and branches diverged? Wha? $ git diff 6b74a277..77939416 Er... this includes a revert of my licence header changes, and, the answer to the test failures, Gabriel's file closing fix: https://github.com/juju/charm/pull/119 https://github.com/juju/charm/pull/120 So, just renaming a variable is *not* safe (if you accidentally back out other changes when merging). Both these regressions happened due to landing 'safe' changes while the branch was broken, and were therefore harder to pin down than they would otherwise have been. Martin -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev