59 bytes, text/x-review-board-request
Bug 1440421 part 2. Mirror https://github.com/mozilla/activity-stream repository layout in browser/extensions/activity-stream.
59 bytes, text/x-review-board-request
Right now, the Activity Stream release scripts flatten parts of https://github.com/mozilla/activity-stream into browser/extensions/activity-stream. In particular, the contents of system-addon in the git repository are lifted into browser/extensions/activity-stream. I'd like to anticipate new approaches to integrating with github and get these two directory structures as close as they can be. That is, I'd like browser/extensions/activity-stream to "be" a git checkout, modulo git metadata. This change looks like hg mv browser/extensions/activity-stream/* browser/extensions/activity-stream/system-addon on the mozilla-central side and the corresponding updates in https://github.com/mozilla/activity-stream/blob/master/yamscripts.yml#L17 on the github side. My immediate motivation is that it's quite difficult for me to test approaches to vendoring node_modules and using the moz.build system with the current layout. That's because the first step is to do a bunch of moves in mozilla-central hg, and then to populate the rest of the git checkout into mozilla-central from my manual checkout. (Note that the git checkout that corresponds to m-c generally isn't master.) The resulting patch does not rebase well at all!
I realize I framed this poorly. It's the package.json that's really needed here. This is all just facilitating using that package.json for (at first) the Webpack generation in the moz.build system. So I should say that the point is that we'd follow-up with [copy package.json from git repository] [copy content-src from git repository] hg add browser/extensions/activity-stream/package.json hg add browser/extensions/activity-stream/system-addon/content-src and then be able to use package.json, with the layout expected by your existing Node-based tooling. If we don't make this repository reflect what's in the git repository, then your existing package.json needs to be reworked. I think that's strictly worse than reworking your mozilla-central structure up front, in anticipation of better days in the future.
dmose had previously discussed moving all of activity-stream repository files into m-c, but I think it was decided against given the mirroring and relatively low friction of existing problems being solved. In particular, this came up in the context of it's a little cumbersome landing simultaneous changes to both m-c and a-s. dmose and I briefly mentioned from a couple github integration meetings ago that because of the proposal, we wouldn't want it copied to m-c… although I don't quite recall why at this moment……
(In reply to Ed Lee :Mardak from comment #2) > dmose had previously discussed moving all of activity-stream repository > files into m-c, but I think it was decided against given the mirroring and > relatively low friction of existing problems being solved. In particular, > this came up in the context of it's a little cumbersome landing simultaneous > changes to both m-c and a-s. Suppose that we weren't checking computed artifacts (bundles, prerenders) into m-c any more. What advantage would there be to not having the same directory structure in a-s and m-c?
I would think if we have npm to generate artifacts as part of build, m-c would end up being the primary repository with a github mirror mostly for PR/review interface. So I guess if that's what we're going to end up with, might as well start moving towards that now? ? Oh, and I think I recall some of the decision against. It was in the context of manifests. Where if we have a github repository, m-c copy, and a m-c vendor manifest; that would have two similar/identical things in m-c. Although maybe we just wouldn't have a manifest as the m-c "copy" would be all that's needed… ?
(In reply to Ed Lee :Mardak from comment #4) > I would think if we have npm to generate artifacts as part of build, m-c > would end up being the primary repository with a github mirror mostly for > PR/review interface. > > So I guess if that's what we're going to end up with, might as well start > moving towards that now? I do think this is the flow that makes most sense for a-s, and yes, this is moving towards that. But even if m-c is not primary -- as now -- having filesystem layouts agree is still a net positive. > Oh, and I think I recall some of the decision against. It was in the context > of manifests. Where if we have a github repository, m-c copy, and a m-c > vendor manifest; that would have two similar/identical things in m-c. > Although maybe we just wouldn't have a manifest as the m-c "copy" would be > all that's needed… ? Any m-c vendor manifest is just metadata describing the relation between a-s in github and some copy (or artifact derived from a copy) of what's in github. So there will always be two versions, github and m-c, and my thrust is that the more those things look the same, the easier the future is.
Well bug 1440284 is yet another example of why we wouldn't want to move the primary repository to m-c yet. If the source is in m-c without npm to run our automation, we would definitely be broken. At least right now, we have some flexibility on when our automation breaks by delaying when we attempt to port these changes. Although without porting, it also generally means we can't export to m-c either. Even without m-c being the primary a-s repository, would a mass change like bug 1440284 have broken our content-src/ too (instead of only breaking lib/ and common/ .jsms)? This particular case, we probably would have avoided problems because we use esmodules in content-src/, so no
See Also: → 1440284
…, so not for this exact change, but there could be some global replacement that might accidentally break our usage of esmodules (as there aren't many uses in m-c currently). Or even more basic is bug 1440284 also broke our eslint because a line was removed and resulted in `no-multiple-empty-lines` failing. Easy to fix sure, but there's a mismatch partially because eslint is disabled for activity-stream in m-c as our .eslintrc has additional plugins.
Nick and I just spent time chatting about this, and it turns out that our (the AS team's) understanding of the proposal wasn't quite what was intended. What I now understand it to be (Nick, please let me know if this wrong!) is that the idea here really is make our export start with (something like) cp -pR of the whole github repo into browser/extensions/activity-stream. This would indeed push most of the files currently in browser/extensions/activity-stream down a level in the directory hierarchy to browser/extensions/activity-stream/system-addon. Nick's take was that the moz.build tweaking necessary to make this go should be straightforward, and he's going to put a more detailed list of the steps that would happen at export time into this bug soon-ish. Nick, I'll put a needinfo on you for now, and once you've added those steps, the AS folks can chat about it a bit more. Feel free to needinfo me once that's done.
https://reviewboard.mozilla.org/r/245760/ is the mozilla-central side of the patch; for whatever reason, it didn't get automatically attached here (nor did it flag khudson for review also, which it should have). Together these two things effectively make mozilla-central a simple mirror of the github repo. Doing local development on the m-c side is not yet implemented (perhaps that should just live in another bug). I'm posting these patches as much for feedback as for review. They might be more-or-less ready to go, or they might want substantial work...
We'll want to export this soon after bug 1466971 export (when the only changes on activity-stream master is this mirroring PR https://github.com/mozilla/activity-stream/pull/4169 )
Depends on: 1466971
Comment on attachment 8983892 [details] Bug 1440421 part 2. Mirror https://github.com/mozilla/activity-stream repository layout in browser/extensions/activity-stream. https://reviewboard.mozilla.org/r/249742/#review256006 Looks good to me!
Attachment #8983892 - Flags: review?(khudson) → review+
Comment on attachment 8983891 [details] Bug 1440421 part 1. Ignore activity-stream and node_modules for linting. https://reviewboard.mozilla.org/r/249740/#review256196 Thanks, lgtm!
Attachment #8983891 - Flags: review?(ahal) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/09a74b229025 part 1. Ignore activity-stream and node_modules for linting. r=ahal https://hg.mozilla.org/integration/autoland/rev/c8aff1ae6322 part 2. Mirror https://github.com/mozilla/activity-stream repository layout in browser/extensions/activity-stream. r=k88hudson
Component: Activity Streams: Newtab → Messaging System
You need to log in before you can comment on or make changes to this bug.