Split download, prebuild check and build phases #535

joachimmetz · 2018-12-27T09:17:52Z

Split download, prebuild check and build phases

codecov · 2018-12-27T09:56:29Z

Codecov Report

Merging #535 into master will decrease coverage by 0.01%.
The diff coverage is 4.22%.

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
- Coverage   26.64%   26.63%   -0.02%     
==========================================
  Files          48       48              
  Lines        4548     4562      +14     
==========================================
+ Hits         1212     1215       +3     
- Misses       3336     3347      +11

Impacted Files	Coverage Δ
l2tdevtools/build_helpers/pkg.py	`0% <0%> (ø)`	⬆️
l2tdevtools/build_helpers/source.py	`0% <0%> (ø)`	⬆️
l2tdevtools/build_helpers/rpm.py	`0% <0%> (ø)`	⬆️
l2tdevtools/build_helpers/msi.py	`0% <0%> (ø)`	⬆️
l2tdevtools/build_helpers/osc.py	`0% <0%> (ø)`	⬆️
l2tdevtools/build_helpers/dpkg.py	`22.36% <4.76%> (-0.27%)`	⬇️
l2tdevtools/build_helpers/interface.py	`90% <50%> (-4.45%)`	⬇️
l2tdevtools/source_helper.py	`21.26% <7.14%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f9058f...baf88fa. Read the comment docs.

Onager

It looks like your branch is out of sync with the log2timeline repo - please rebase

Onager · 2018-12-27T12:38:13Z

l2tdevtools/build_helpers/dpkg.py

    return missing_packages

+  def CheckProjectConfiguration(self):
+    """Checks if the project configuration.


Looks like the sentence got cut off here, please fix.

Onager · 2019-01-16T19:01:18Z

l2tdevtools/build_helpers/interface.py

@@ -54,3 +54,11 @@ def CheckBuildRequired(self, unused_source_helper_object):
      bool: True if a build is required, False otherwise.
    """
    return True
+
+  def CheckProjectConfiguration(self):
+    """Checks if the project configuration.


Please complete the docstring.

l2tdevtools/source_helper.py

l2tdevtools/build_helpers/dpkg.py

l2tdevtools/build_helpers/rpm.py

Onager · 2019-01-16T19:29:33Z

tools/build.py


-  def _BuildProject(self, download_helper_object, project_definition):
-    """Builds a project.
+  def _BuildProjectForDistribution(


Please update the docstring, and perhaps rename to method to indicate that this also builds projects for which there is no distribution. Could you refactor to make distribution an optional arg, to make this clearer?

Let's do not do an additional refactor in this CL, seeing it already affects a significant part of the code base.

Also your comment sounds to me like ambiguity on the semantics of "distribution" let's have this discussion outside of this CL before making changes.

This method is being added in this PR, so I don't know what you mean by "additional refactor". The refactor is happening here, now.

I don't know what you mean about the semantics of distribution - per the docstring this is the "name of the distribution", and is None for every build target except dpkg-source.

ack, per conversation, which was not clear from the initial review comment, reverted the name change of the method

Onager · 2019-01-16T19:45:40Z

tools/build.py

+  for disabled_package in disabled_projects:
+    undefined_projects.remove(disabled_package)
+
+  check_configuration = set()


Rename to misconfigured_builds to be consistent with the other sets here.

Onager · 2019-01-16T19:45:44Z

tools/build.py

+          missing_build_dependencies.update(dependencies)
+
+        if not project_builder.CheckProjectConfiguration(project_definition):
+          print('Detected inconsistency in configuration of: {0:s}'.format(


Isn't this "detected error in configuration of: {0:s}"? I don't know what the configuration is supposed to be consistent with.

if that is more clear to you, I can change inconsistency into error

Onager · 2019-01-16T19:46:39Z

tools/build.py


-    logging.info('Processing: {0:s}'.format(project_definition.name))
+        print('Failed downloading: {0:s}'.format(project_definition.name))
+        failed_downloads.add(project_definition.name)


Why continue here? If a project can't be downloaded, don't we want to abort by default?

Nope, build.py is supposed to continue with other builds. What happens is the next steps (build prep and build) for that project are skipped instead.

tools/build.py

Onager · 2019-01-17T12:40:16Z

tools/build.py

-          source_helper_object.project_name,
-          build_helper_object.LOG_FILENAME)
+  def CheckProjectConfiguration(self, project_definition):
+    """Checks if the project configuration.


This hasn't been addressed.

Onager · 2019-01-17T12:40:18Z

tools/build.py

-      self, build_helper_object, source_helper_object, distribution):
-    """Builds a project for a specific distribution.
+  def CheckBuildDependencies(self, project_definition):
+    """Checks if the build dependencies a project are met.


This hasn't been addressed.

Onager · 2019-01-17T12:40:25Z

tools/build.py

@@ -178,7 +192,29 @@ def Build(self, project_definition):
      raise ValueError('Unsupported download URL: {0:s}.'.format(
          project_definition.download_url))

-    return self._BuildProject(download_helper_object, project_definition)
+    # TODO: implement


This hasn't been addressed.

tools/build.py

Onager · 2019-01-17T12:50:24Z

tools/build.py


-  def _BuildProject(self, download_helper_object, project_definition):
-    """Builds a project.
+  def _BuildProjectForDistribution(


This method is being added in this PR, so I don't know what you mean by "additional refactor". The refactor is happening here, now.

I don't know what you mean about the semantics of distribution - per the docstring this is the "name of the distribution", and is None for every build target except dpkg-source.

Onager

It looks your branch has diverged from master - can you rebase please?

joachimmetz requested a review from Onager December 27, 2018 09:17

joachimmetz assigned Onager Dec 27, 2018

joachimmetz mentioned this pull request Dec 27, 2018

Improvements to build script #3

Open

29 tasks

joachimmetz mentioned this pull request Dec 27, 2018

Added Python 3 dpkg build dependency check #540

Merged

joachimmetz force-pushed the master branch from 6edb0a7 to ae6cede Compare December 27, 2018 21:40

Onager requested changes Jan 4, 2019

View reviewed changes

joachimmetz force-pushed the bsplit branch from 240863f to 1626e4a Compare January 4, 2019 14:08

joachimmetz requested a review from Onager January 4, 2019 14:08

joachimmetz force-pushed the bsplit branch from 1626e4a to 4eeea76 Compare January 13, 2019 11:34

This was referenced Jan 13, 2019

Changes to auto-detect build system #549

Merged

Changes to determine dpkg build configuration based on setup.py install #551

Merged

Updates for Timesketch and clean up #555

Merged

joachimmetz force-pushed the bsplit branch from 4eeea76 to 1fbef2c Compare January 16, 2019 19:20

Onager requested changes Jan 16, 2019

View reviewed changes

joachimmetz force-pushed the bsplit branch from 1fbef2c to 55c2308 Compare January 17, 2019 05:37

joachimmetz requested a review from Onager January 17, 2019 05:37

Onager requested changes Jan 17, 2019

View reviewed changes

joachimmetz requested a review from Onager January 17, 2019 20:02

joachimmetz force-pushed the master branch from a510a9e to 8f9058f Compare January 17, 2019 20:09

Onager requested changes Jan 18, 2019

View reviewed changes

joachimmetz added 4 commits January 18, 2019 12:20

Split download, prebuild check and build phases

29ff1e1

Changes after review

c2f6943

Changes after review

5c0779b

Changes after review

baf88fa

joachimmetz force-pushed the bsplit branch from c24694e to baf88fa Compare January 18, 2019 11:20

joachimmetz requested a review from Onager January 18, 2019 11:28

Onager approved these changes Jan 18, 2019

View reviewed changes

Onager merged commit 17e583e into log2timeline:master Jan 18, 2019

joachimmetz deleted the bsplit branch January 18, 2019 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split download, prebuild check and build phases #535

Split download, prebuild check and build phases #535

joachimmetz commented Dec 27, 2018

codecov bot commented Dec 27, 2018 •

edited

Loading

Onager left a comment

Onager Dec 27, 2018

joachimmetz Jan 4, 2019

Onager Jan 16, 2019

joachimmetz Jan 17, 2019

Onager Jan 16, 2019

joachimmetz Jan 17, 2019 •

edited

Loading

Onager Jan 17, 2019

joachimmetz Jan 17, 2019

Onager Jan 16, 2019

joachimmetz Jan 17, 2019

Onager Jan 16, 2019

joachimmetz Jan 17, 2019

Onager Jan 16, 2019

joachimmetz Jan 17, 2019

Onager Jan 17, 2019

Onager Jan 17, 2019

Onager Jan 17, 2019

Onager Jan 17, 2019

Onager left a comment

Split download, prebuild check and build phases #535

Split download, prebuild check and build phases #535

Conversation

joachimmetz commented Dec 27, 2018

codecov bot commented Dec 27, 2018 • edited Loading

Codecov Report

Onager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joachimmetz Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Onager left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 27, 2018 •

edited

Loading

joachimmetz Jan 17, 2019 •

edited

Loading