Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2402423002:
Enable saving the same file in multiple archives (Closed)
|
Created:
4 years, 2 months ago by chenwilliam Modified:
4 years, 2 months ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionEnable saving the same file in multiple archives
Earlier I submitted a CL to archive content shell and I ran into an issue
where the current FILES.cfg parser doesn't allow listing the same file
for multiple archives.
I opted to allow multiple entries of the same file in FILES.cfg rather than
making the 'archive' property a list because this allows more configurability
(e.g. for icudtl.dat, we only need to archive it for content shell for dev builds
but for the main archive we archive it for dev and official builds).
BUG=654486
Committed: https://chromium.googlesource.com/chromium/tools/build/+/47c03a95aef64407f6d9bb5db7509bdca9be47f4
Committed: https://chromium.googlesource.com/chromium/tools/build/+/346ca76f9663a759d1caa23c04beaca19be8932f
Patch Set 1 #Patch Set 2 : Remove accidental commit #
Total comments: 4
Patch Set 3 : Nits #Patch Set 4 : Follow-up fix broken build #Messages
Total messages: 22 (13 generated)
The CQ bit was checked by chenwilliam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Implement logic to save the same file in multiple archives Earlier I submitted a CL to archive content shell and I ran into an issue where the current FILES.cfg parser doesn't allow listing the same file for multiple archives. I opted to allow multiple entries of the same file in FILES.cfg rather than making the 'archive' property a list because this allows more configurability (e.g. for icudtl.dat, we only need to archive it for content shell for dev builds but for the main archive we archive it for dev and official builds). BUG=654486 ========== to ========== Enable saving the same file in multiple archives Earlier I submitted a CL to archive content shell and I ran into an issue where the current FILES.cfg parser doesn't allow listing the same file for multiple archives. I opted to allow multiple entries of the same file in FILES.cfg rather than making the 'archive' property a list because this allows more configurability (e.g. for icudtl.dat, we only need to archive it for content shell for dev builds but for the main archive we archive it for dev and official builds). BUG=654486 ==========
chenwilliam@chromium.org changed reviewers: + kerz@chromium.org, mmoss@chromium.org
Michael - PTAL. I saw that you worked on this file a while ago so I thought you'd be a good reviewer. Kerz - Once this lands, I think I can safely re-land https://codereview.chromium.org/2383043003/
lgtm https://codereview.chromium.org/2402423002/diff/20001/scripts/common/archive_... File scripts/common/archive_utils.py (left): https://codereview.chromium.org/2402423002/diff/20001/scripts/common/archive_... scripts/common/archive_utils.py:167: Nit: Keep 2 lines before top-level functions/classes. https://codereview.chromium.org/2402423002/diff/20001/scripts/common/archive_... File scripts/common/archive_utils.py (right): https://codereview.chromium.org/2402423002/diff/20001/scripts/common/archive_... scripts/common/archive_utils.py:142: return has_file Maybe simplify slightly by getting rid of the 'has_file' and just doing: for fileobj in self._files_list: if fileobj['filename'] == filename and self._buildtype in fileobj['optional']: return True return False
https://codereview.chromium.org/2402423002/diff/20001/scripts/common/archive_... File scripts/common/archive_utils.py (right): https://codereview.chromium.org/2402423002/diff/20001/scripts/common/archive_... scripts/common/archive_utils.py:142: return has_file On 2016/10/11 18:23:32, Michael Moss wrote: > Maybe simplify slightly by getting rid of the 'has_file' and just doing: > > for fileobj in self._files_list: > if fileobj['filename'] == filename and self._buildtype in > fileobj['optional']: > return True > return False Ugh, my logic was backwards, ignore. We want it to be required if any instance of the file is required.
https://codereview.chromium.org/2402423002/diff/20001/scripts/common/archive_... File scripts/common/archive_utils.py (left): https://codereview.chromium.org/2402423002/diff/20001/scripts/common/archive_... scripts/common/archive_utils.py:167: On 2016/10/11 18:23:32, Michael Moss wrote: > Nit: Keep 2 lines before top-level functions/classes. Done.
The CQ bit was checked by chenwilliam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmoss@chromium.org Link to the patchset: https://codereview.chromium.org/2402423002/#ps40001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enable saving the same file in multiple archives Earlier I submitted a CL to archive content shell and I ran into an issue where the current FILES.cfg parser doesn't allow listing the same file for multiple archives. I opted to allow multiple entries of the same file in FILES.cfg rather than making the 'archive' property a list because this allows more configurability (e.g. for icudtl.dat, we only need to archive it for content shell for dev builds but for the main archive we archive it for dev and official builds). BUG=654486 ========== to ========== Enable saving the same file in multiple archives Earlier I submitted a CL to archive content shell and I ran into an issue where the current FILES.cfg parser doesn't allow listing the same file for multiple archives. I opted to allow multiple entries of the same file in FILES.cfg rather than making the 'archive' property a list because this allows more configurability (e.g. for icudtl.dat, we only need to archive it for content shell for dev builds but for the main archive we archive it for dev and official builds). BUG=654486 Committed: https://chromium.googlesource.com/chromium/tools/build/+/47c03a95aef64407f6d9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/tools/build/+/47c03a95aef64407f6d9...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2407183003/ by alph@chromium.org. The reason for reverting is: Broke the archive build step https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/26845.
Message was sent while issue was closed.
Description was changed from ========== Enable saving the same file in multiple archives Earlier I submitted a CL to archive content shell and I ran into an issue where the current FILES.cfg parser doesn't allow listing the same file for multiple archives. I opted to allow multiple entries of the same file in FILES.cfg rather than making the 'archive' property a list because this allows more configurability (e.g. for icudtl.dat, we only need to archive it for content shell for dev builds but for the main archive we archive it for dev and official builds). BUG=654486 Committed: https://chromium.googlesource.com/chromium/tools/build/+/47c03a95aef64407f6d9... ========== to ========== Enable saving the same file in multiple archives Earlier I submitted a CL to archive content shell and I ran into an issue where the current FILES.cfg parser doesn't allow listing the same file for multiple archives. I opted to allow multiple entries of the same file in FILES.cfg rather than making the 'archive' property a list because this allows more configurability (e.g. for icudtl.dat, we only need to archive it for content shell for dev builds but for the main archive we archive it for dev and official builds). BUG=654486 Committed: https://chromium.googlesource.com/chromium/tools/build/+/47c03a95aef64407f6d9... ==========
The CQ bit was checked by chenwilliam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmoss@chromium.org Link to the patchset: https://codereview.chromium.org/2402423002/#ps60001 (title: "Follow-up fix broken build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enable saving the same file in multiple archives Earlier I submitted a CL to archive content shell and I ran into an issue where the current FILES.cfg parser doesn't allow listing the same file for multiple archives. I opted to allow multiple entries of the same file in FILES.cfg rather than making the 'archive' property a list because this allows more configurability (e.g. for icudtl.dat, we only need to archive it for content shell for dev builds but for the main archive we archive it for dev and official builds). BUG=654486 Committed: https://chromium.googlesource.com/chromium/tools/build/+/47c03a95aef64407f6d9... ========== to ========== Enable saving the same file in multiple archives Earlier I submitted a CL to archive content shell and I ran into an issue where the current FILES.cfg parser doesn't allow listing the same file for multiple archives. I opted to allow multiple entries of the same file in FILES.cfg rather than making the 'archive' property a list because this allows more configurability (e.g. for icudtl.dat, we only need to archive it for content shell for dev builds but for the main archive we archive it for dev and official builds). BUG=654486 Committed: https://chromium.googlesource.com/chromium/tools/build/+/47c03a95aef64407f6d9... Committed: https://chromium.googlesource.com/chromium/tools/build/+/346ca76f9663a759d1ca... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/tools/build/+/346ca76f9663a759d1ca... |
