feat: add python as a supported build tool by sophie-bates · Pull Request #67 · oracle/macaron

sophie-bates · 2023-02-15T10:03:28Z

Currently able to detect whether a Python project uses Pip or Poetry to manage its dependencies. This should be expanded in the future.

For Pip projects, we search for the existence of apyproject.toml, setup.py, or setup.cfg file.
For Poetry projects, we search for the existence of a pyproject.toml file, that we parse using tomllib to search for configuration settings for [tools.poetry].

For the Build Service Check, the build commands that we currently support are:

pip install
poetry build
flit build
python setup.py

For the Build as Code Check, the deploy commands that we currently support are:

poetry publish
flit publish
twine upload

As well as the use of the pypa/gh-action-pypi-publish reusable workflow.

behnazh-w · 2023-02-17T00:11:23Z

+
+"""This module contains the Pip class which inherits BaseBuildTool.
+
+This module is used to work with repositories that use Poetry for dependency management.


Suggested change

This module is used to work with repositories that use Poetry for dependency management.

This module is used to work with repositories that use pip for dependency management.

Fixed in f7bec7f

behnazh-w · 2023-02-17T00:14:00Z

+    pattern = os.path.join(path, "**", file_name)
+    files_detected = glob.glob(pattern, recursive=True)


I'm not sure if adding this wrapper function over glob.glob makes too much sense. All it's doing is calling glob.glob() really and I think this could just be done directly at is_detected function.

I agree. I've removed this function and put the functionality inside is_detected instead in f7bec7f

behnazh-w · 2023-02-17T03:44:45Z

+            if files_detected:
+                try:
+                    # Take the highest level file (shortest file path)
+                    file_path = min(files_detected, key=len)


Note that the len of the first level path part could be larger than a path with more parts, e.g., a/b/c.txt vs aaaaaaaaaaaa/c.txt. You would need to get the number of parts here probably:

len(Path(x).parts)

Good point, I changed the key function to look for parts instead in f7bec7f

behnazh-w · 2023-02-17T03:51:50Z

+    def prepare_config_files(self, wrapper_path: str, build_dir: str) -> bool:
+        """Prepare the necessary wrapper files for running the build.
+
+        This method will return False if there is any errors happened during operation.


Suggested change

This method will return False if there is any errors happened during operation.

This method returns False on errors.

Fixed in f7bec7f

behnazh-w · 2023-02-17T03:52:21Z

+        Returns
+        -------
+        bool
+            True if succeed else False.


Suggested change

True if succeed else False.

True if succeeds else False.

Fixed in f7bec7f

sophie-bates · 2023-03-17T04:45:34Z

-                        justification: list[str | dict[str, str]] = [
+                        justification_cmd: list[str | dict[str, str]] = [
                            {
                                f"The target repository uses build tool {build_tool.name} to deploy": bash_source_link,


For Python projects, rather than saying (i.e.) "...uses build tool pip to deploy", should this reflect the actual publishing tool used, to be more descriptive? Something like: "...uses build tool pip, with Twine to deploy".

For Python projects, rather than saying (i.e.) "...uses build tool pip to deploy", should this reflect the actual publishing tool used, to be more descriptive? Something like: "...uses build tool pip, with Twine to deploy".

A more descriptive justification would be good, but the original message was logged with the assumption that the command would already contain the exact build tool and deploy command. Please feel free to adjust the message and push for review if you have a better message in mind.

behnazh-w · 2023-03-21T23:13:14Z

+    twine
+    flit
+    conda
+builder_module =


Can you please add a description for this attribute. It might not be straightforward for someone who is new to our codebase.

Done in 9c167f2.

behnazh-w · 2023-03-21T23:23:04Z

+        -------
+        bool
+            True if succeed else False.
+        """


Can you please add a comment that pip does not require any preparation?

Done in 9c167f2.

behnazh-w · 2023-03-21T23:23:32Z

+        DependencyAnalyzer
+            The DependencyAnalyzer object.
+        """
+        return NoneDependencyAnalyzer()


Can you please add a TODO to implement it later?

Done in 9c167f2.

behnazh-w · 2023-03-21T23:25:39Z

+        bool
+            True if succeeds else False.
+        """
+        return False


Why are we returning False here? Also please add a comment if preparation is not necessary for poetry.

Fixed in 9c167f2.

behnazh-w · 2023-03-21T23:25:50Z

+        DependencyAnalyzer
+            The DependencyAnalyzer object.
+        """
+        return NoneDependencyAnalyzer()


Same comment as pip.

Done in 9c167f2.

behnazh-w · 2023-03-22T00:47:39Z

        gradle_deploy.dynamic_data["ci_services"] = [ci_info]
        assert check.run_check(gradle_deploy, check_result) == CheckResultType.PASSED

+        # Use poetry publish to publish the artifact


Suggested change

# Use poetry publish to publish the artifact

# Use poetry publish to publish the artifact.

Same for comments below.

behnazh-w · 2023-03-22T00:50:05Z

        maven_deploy.dynamic_data["ci_services"] = [ci_info]
        assert check.run_check(maven_deploy, check_result) == CheckResultType.FAILED
+
+        # This Github Actions workflow uses gh-action-pypi-publish to publish the artifact.


Can you please add a new function to test GitHub Actions workflow deployment? This function is getting hard to read.

Completed in 9c167f2. Refactored this file to use functions (+ fixtures) rather than the class-based approach. I added several fixtures for the build tools and CI services into conftest.py so they can be shared across many tests.

behnazh-w · 2023-03-22T00:51:12Z

+        assert check.run_check(pip_module_build_ci, check_result) == CheckResultType.FAILED
+
+        # Use pip as a module in CI with invalid goal to build the artifact.
+        pip_module_build_ci = AnalyzeContext("use_build_tool", os.path.abspath("./"), MagicMock())


When you adjust the attribute names in the spec, please update the variable names here too.

Done in 9c167f2.

behnazh-w · 2023-03-22T00:51:36Z

        maven_build_ci.dynamic_data["ci_services"] = [ci_info]
        assert check.run_check(maven_build_ci, check_result) == CheckResultType.FAILED
+
+        # TODO: Python module - maybe not for this context, just build


Can you please elaborate?

This was meant to be removed, done in 9c167f2.

behnazh-w · 2023-03-22T00:54:39Z

@sophie-bates Please update the README.md and add Python support (minus dependency analysis for now).

tromai · 2023-03-27T23:36:53Z

+            cfg_path = next(list_iter)
+            yield Path(cfg_path).parent.relative_to(repo_path)
+            while next_item := next(list_iter):
+                if str(Path(cfg_path).parent) in next_item:


I would recommend we very careful of the in between two strings because the behavior might be implicit. For example, if cfg_path is /home/boo and next_item is /home/foo/home/boo/ , if str(Path(cfg_path).parent) in next_item: will be True and we will ignore the valid /home/foo/home/boo/.
I think we could use .startswith here ?

That's a good point. I based this abstraction off the implementation in BaseBuildTool.get_build_dirs(), so this function would need updating too if that's the case.

@behnazh-w How do you think about it?

That's a good point. I based this abstraction off the implementation in BaseBuildTool.get_build_dirs(), so this function would need updating too if that's the case.

That's not possible because first we sort the elements based on the parent path. So /home/foo/home/boo/ should never be filtered by /home/boo. But using .startswith would make it more explicit.

Regardless, I'm not sure if it makes sense to override this function to add config_files = self.build_configs + self.package_lock. As discussed, poetry.lock on its own cannot determine a build tool and pyproject.toml must always exist for poetry builds. So there shouldn't be any need to add self.package_lock here.

@behnazh-w that's a good point. I had done it this way before we adjusted the config variables, and now I agree that we don't need to override.

Removed this override in 29cdd3a.

tromai · 2023-03-27T23:50:18Z

+
+    Parameters
+    ----------
+    setup_test


The doc is inconsistent with the parameter provided to this fixture.

I think this fixture should be passed the setup_test parameter.

Fixed in 29cdd3a.

behnazh-w · 2023-03-30T04:18:48Z

+# ---
+# name: test_get_build_dirs[mock_repo1]
+  list([
+    PosixPath('.'),


This seems to be a false positive. The problem is that get_build_dirs function is not checking if the detected directory has a valid build. This PR should fix this issue: #135

Thank you for fixing that.

behnazh-w · 2023-03-30T04:26:52Z

-        maven_deploy.dynamic_data["ci_services"] = [ci_info]
-        assert check.run_check(maven_deploy, check_result) == CheckResultType.FAILED
+
+@pytest.fixture()


This fixture is not needed. Why not to directly call BuildAsCodeCheck() from the test function if nothing needs to be prepared before the test?

Fixed in 29cdd3a.

behnazh-w · 2023-03-30T04:29:45Z

+    CheckResult
+        The CheckResult instance.
+    """
+    return CheckResult(justification=[])  # type: ignore


It's better to instantiate CheckResult directly in tests to set the expected output. This fixture is not doing much and I'm not sure if it's helpful.

Fixed in 29cdd3a.

behnazh-w · 2023-03-30T04:34:14Z

+    root.add_callee(callee)
+    github_actions_service.build_call_graph_from_node(callee)
+    ci_info["callgraph"] = gh_cg
+    assert build_as_code_check.run_check(gha_deploy, check_result) == CheckResultType.PASSED


Please add a failed case too, for example change the workflow name to None and a wrong name and check that they fail.

Fixed in 29cdd3a.

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

…as_code and build_service checks Signed-off-by: sophie-bates <sophie.bates@oracle.com>

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

oracle-contributor-agreement Bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 15, 2023

sophie-bates changed the title ~~Add python as a supported build tool~~ feat: add python as a supported build tool Feb 15, 2023

sophie-bates requested review from behnazh-w and tromai February 15, 2023 22:30

tromai reviewed Feb 16, 2023

View reviewed changes

Comment thread tests/e2e/expected_results/urllib3/urllib3.json Outdated

sophie-bates force-pushed the add-python-as-a-supported-build-tool branch from 20cca24 to c7c1eef Compare February 16, 2023 07:04

behnazh-w reviewed Feb 17, 2023

View reviewed changes

sophie-bates mentioned this pull request Feb 21, 2023

Add support for detecting python deployments that use Github Actions #73

Closed

sophie-bates linked an issue Mar 17, 2023 that may be closed by this pull request

Add support for detecting python deployments that use Github Actions #73

Closed

sophie-bates force-pushed the add-python-as-a-supported-build-tool branch from 9ad67a9 to 98eaf59 Compare March 17, 2023 01:38

sophie-bates commented Mar 17, 2023

View reviewed changes

tromai reviewed Mar 17, 2023

View reviewed changes

behnazh-w reviewed Mar 22, 2023

View reviewed changes

tromai reviewed Mar 27, 2023

View reviewed changes

behnazh-w mentioned this pull request Mar 28, 2023

chore: improve path comparison in build dir detection #123

Merged

behnazh-w reviewed Mar 30, 2023

View reviewed changes

sophie-bates force-pushed the add-python-as-a-supported-build-tool branch from 29cdd3a to 52570e7 Compare April 4, 2023 01:08

behnazh-w approved these changes Apr 4, 2023

View reviewed changes

tromai approved these changes Apr 4, 2023

View reviewed changes

sophie-bates added 7 commits April 4, 2023 14:04

feat: add checks to detect pip and poetry projects

e4fab89

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

chore: update expected results for urllib3

09926fc

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

chore: update expected results for urllib3 to match integration_tests.sh

de62f78

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

test: add tests for poetry build tool

6f7d867

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

fix: remove submodules from mock poetry repos

b52061f

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

chore: address PR comments

9527cd5

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

feat: add support for python packaging and publishing tools in build_…

f9555d7

…as_code and build_service checks Signed-off-by: sophie-bates <sophie.bates@oracle.com>

sophie-bates added 7 commits April 4, 2023 14:04

chore: update expected result for urllib3 test

9173323

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

feat: add build_service and build_as_code tests for pip and poetry

343612e

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

feat: add test for trusted GH deploy action in build_as_code check

2be2612

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

chore: respond to PR review comments.

da5cc6a

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

fix: update macaron_path value used in test_build_as_code-check

67fc25a

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

chore: address PR review comments

5b27054

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

test: update poetry build dir snapshot

8165079

Signed-off-by: sophie-bates <sophie.bates@oracle.com>

sophie-bates force-pushed the add-python-as-a-supported-build-tool branch from 52570e7 to 8165079 Compare April 4, 2023 04:07

sophie-bates merged commit 0e961be into staging Apr 4, 2023

sophie-bates deleted the add-python-as-a-supported-build-tool branch April 4, 2023 04:54

tromai mentioned this pull request Aug 17, 2023

feat: add docker build detection #409

Merged

art1f1c3R pushed a commit that referenced this pull request Nov 29, 2024

feat: add python as a supported build tool (#67)

a32a231

Signed-off-by: sophie-bates <sophie.bates@oracle.com>


		"""This module contains the Pip class which inherits BaseBuildTool.

		This module is used to work with repositories that use Poetry for dependency management.

		pattern = os.path.join(path, "**", file_name)
		files_detected = glob.glob(pattern, recursive=True)

	This method will return False if there is any errors happened during operation.
	This method returns False on errors.

	# Use poetry publish to publish the artifact
	# Use poetry publish to publish the artifact.

Conversation

sophie-bates commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophie-bates Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behnazh-w commented Mar 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophie-bates commented Feb 15, 2023 •

edited

Loading

sophie-bates Mar 17, 2023 •

edited

Loading

sophie-bates Mar 28, 2023 •

edited

Loading