Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: cyclic dependency error in Python distribution of projen #811

Closed
Chriscbr opened this issue May 19, 2021 · 11 comments · Fixed by #1286
Closed

bug: cyclic dependency error in Python distribution of projen #811

Chriscbr opened this issue May 19, 2021 · 11 comments · Fixed by #1286

Comments

@Chriscbr
Copy link
Contributor

Since projen is built with jsii, it is also vended as a python library on pypi (see here).

However, it seems that using this module to synthesize even simple projects isn't possible due to cyclic dependencies. For example, if we install projen with pip and write a project definition in .projenrc.py like this:

from projen import Project

project = Project()

project.synth()

Then running python .projenrc.py throws the following error:

Traceback (most recent call last):
  File "/Users/rybickic/Developer/projen-test/.projenrc.py", line 3, in <module>
    from projen import Project
  File "/Users/rybickic/Developer/projen-test/.env/lib/python3.9/site-packages/projen/__init__.py", line 323, in <module>
    from .deps import Dependencies as _Dependencies_53ce8fa0
  File "/Users/rybickic/Developer/projen-test/.env/lib/python3.9/site-packages/projen/deps/__init__.py", line 13, in <module>
    from .. import Component as _Component_2b0ad27f, Project as _Project_57d89203
ImportError: cannot import name 'Component' from partially initialized module 'projen' (most likely due to a circular import) (/Users/rybickic/Developer/projen-test/.env/lib/python3.9/site-packages/projen/__init__.py)

My best guess of the cause of this is that Project directly references Component in several of its methods, and likewise Project references Component in its constructor.

Is it possible we could resolve this cycle by changing one of these to an interface like IProject or IComponent instead?

I'm also curious how other JSII libraries like the AWS CDK go about avoiding (or resolving) cyclic dependencies like this.


Note: since this is just the first error that's thrown, it's not clear fixing this alone would be enough to get .projenrc.py working in full. When I ran madge --circular --extensions ts src it reports there are upwards of 45 potential circular dependencies as of posting (see below) -- though it's possible not all of these are relevant to synthesizing Python projects.

1) component.ts > project.ts > clobber.ts
2) project.ts > clobber.ts
3) component.ts > project.ts
4) file.ts > _resolve.ts
5) component.ts > project.ts > file.ts
6) project.ts > file.ts
7) component.ts > project.ts > gitpod.ts
8) project.ts > gitpod.ts
9) project.ts > gitpod.ts > yaml.ts > object-file.ts
10) project.ts > gitpod.ts > yaml.ts
11) yaml.ts
12) project.ts > ignore-file.ts
13) project.ts > json.ts
14) component.ts > project.ts > logger.ts
15) project.ts > logger.ts
16) project.ts > readme.ts
17) component.ts > project.ts > readme.ts > sample-file.ts
18) project.ts > readme.ts > sample-file.ts
19) component.ts > project.ts > tasks/tasks.ts
20) project.ts > tasks/tasks.ts
21) tasks/tasks.ts > tasks/task.ts
22) semver.ts
23) github/github.ts > github/dependabot.ts
24) github/mergify.ts > github/github.ts
25) github/github.ts > github/pr-template.ts
26) github/github.ts > github/workflows.ts
27) node-project.ts > javascript/projenrc.ts
28) node-project.ts > jest.ts
29) node-project.ts > jest.ts > typescript-config.ts
30) node-project.ts > upgrade-dependencies.ts
31) node-project.ts > version.ts
32) typescript.ts > typescript-typedoc.ts
33) typescript.ts > typescript/projenrc.ts
34) jsii-project.ts > jsii-docgen.ts
35) ini.ts
36) util/semver.ts
37) vscode/launch-config.ts > vscode/vscode.ts
38) python/pip.ts > python/python-project.ts
39) python/poetry.ts > python/python-packaging.ts
40) python/python-project.ts > python/poetry.ts
41) python/python-project.ts > python/pytest.ts
42) python/python-project.ts > python/python-sample.ts
43) python/python-project.ts > python/setuptools.ts
44) python/python-project.ts > python/setuptools.ts > python/setuppy.ts
45) python/python-project.ts > python/venv.ts
mergify bot pushed a commit that referenced this issue May 19, 2021
Actual synthesis with `.projenrc.py` is currently not functional because of #811, but this commit adds the CLI flag along with projenrc.py file generation so the feature can be experimentally tested going forward.

Since it's still not functional, I did not update the docs to mention `.projenrc.py` since it would probably cause more confusion than help.

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@eganjs
Copy link
Contributor

eganjs commented May 25, 2021

Having looked into the generated code a bit I believe that is the issue is src/project.ts (which translates to projen/__init__.py) importing src/deps/dependencies.ts (which translates into projen/deps/__init__.py) which then imports src/component.ts (which translates into projen/__init__.py) thus creating a cyclic import.

The (perhaps unfavorable) way to fix this immediate issue would be to move src/deps/dependencies.ts up to the src directory, which would cause jsii to put it into projen/__init__.py.

@eladb
Copy link
Contributor

eladb commented May 25, 2021

We need to restructure the files into submodules that don't take any cyclic dependencies. I was wondering if there is some eslint rule that can help with that.

@github-actions
Copy link
Contributor

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon.

@github-actions github-actions bot added the stale label Jul 25, 2021
@Chriscbr
Copy link
Contributor Author

Keep open

@github-actions github-actions bot removed the stale label Jul 26, 2021
@github-actions
Copy link
Contributor

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon.

@github-actions github-actions bot added the stale label Sep 25, 2021
@Chriscbr
Copy link
Contributor Author

Keep

@github-actions github-actions bot removed the stale label Sep 26, 2021
@github-actions
Copy link
Contributor

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon.

@github-actions github-actions bot added the stale label Nov 26, 2021
@Chriscbr Chriscbr removed the stale label Nov 26, 2021
@Chriscbr
Copy link
Contributor Author

@eladb Perhaps the eslint import/no-cycle rule could be used? It would actually report more errors than we care about since it would report cycles between files in the same jsii submodule - but it's still a good starting point.

I was revisiting this issue briefly, just focusing on the dependency cycles including Project as a starting point. I think there are three kinds of solutions that would in theory fix any given dependency cycle:

  1. move the component(s) to be in the same jsii submodule as Project
  2. change Project to not depend on the component(s)
  3. change the component(s) to not depend on Project

Option 1) seems weird at first, but I wonder if it might be the most pragmatic. We could keep source code files in subdirectories for organizational purposes, but export them not as named submodules. I'm not familiar enough with jsii submodules to understand what the drawbacks of this would be, besides having to worry a little bit about naming collisions.

Option 2) could be achieved by removing all public fields/methods on Project relating to specific components (like project.deps, addTask, annotateGenerated...), not initializing any components within Project, and instead requiring components to be accessed (and auto-initialized) via Xxx.of(project) APIs. I feel like this might decrease the discoverability of components through IDE autocomplete, and introduce more complexity for users looking to author their own components, so I'm not sure if it's ideal.

Option 3) I'm not how this could be achieved. I thought of having components depend on an IProject interface, but I think this would only make the cycles longer?

Curious what you think about these options?

@eladb
Copy link
Contributor

eladb commented Nov 29, 2021

There are two aspects here: (1) how do we enforce that submodules do not have cyclic dependencies; and (2) how do we untangle the mess we have in projen today.

For (1) I think we need a robust long-term solution to this problem, and I think the solution should be in the JSII compiler. One of the tenets of JSII is that the compiler should not allow you to write code that will not be translatable to other languages. This is a good example of a missing check in the JSII compiler. Let's explore with @RomainMuller what it would take to add such a check (likely under a feature flag at this point given its a breaking change).

For (2) I think you are on the right direction - basically this requires a redesign, especially of the low-level types (Component, Project, FileBase). I am generally on board with moving all the "core" types under a single submodule (the root) and then layer the rest of top of it. I've been playing with import/no-cycle and started doing that a few times but it always pulls an infinite thread of cycles. Ho my.. (it's not the first time I've been de-spagettizing a project, and it's never fun).

Here's a wild idea: create a new library called projen/projen-core and move all the core types in there (without submodules). Once we've untangled the core pieces I think it will be easier to maintain the layers within the rest of the system.

WDYT?

@Chriscbr
Copy link
Contributor Author

Agree with (1), this should be compile-time checked in JSII.

For (2) I'm OK with moving pieces into a separate library, though it's not immediately clear to me how that makes the de-spagettizing easier.

(There are could be other reasons to split the code into multiple repos, but I think it also has some costs - e.g. all else equal, it's a better contributor experience when the code is all in a single repo).

I am generally on board with moving all the "core" types under a single submodule (the root) and then layer the rest of top of it.

Cool - "anything referenced by Project should be in core" seems like a good heuristic to start on, this is something I can work with.

@eladb
Copy link
Contributor

eladb commented Nov 29, 2021

I guess you are right that a separate library is just a technique to enforce the layering and we should have an in-module way of doing it.

If we can get the linter to help us enforce it in-module, then let's start collecting all of the core stuff under a core submodule.

I am wondering if maybe this is our version of mono-repos. Can we do something like have a package.json file in each submodule that declares its local dependencies and use that to enforce the layering within the module. I recall some import magic that helps you declare local references.... I think we might be able to utilize TypeScript Path Mapping for this

@mergify mergify bot closed this as completed in #1286 Dec 5, 2021
mergify bot pushed a commit that referenced this issue Dec 5, 2021
Fixes #811 

Resolves the cyclic dependency errors by reorganizing the submodules within projen so that there are no more import cycles (between submodules). This requires changing the location of MANY classes and associated types. This enables us to add support in later PRs for writing projenrc files in Python and Go.

In TypeScript projects, classes and types should be imported in one of two ways:

```ts
// method 1
import { awscdk } from "projen";

const project = new awscdk.AwsCdkConstructLibrary({ ... });

// method 2
import { AwsCdkConstructLibrary } from "projen/awscdk";

const project = new AwsCdkConstructLibrary({ ... });
```

Due to these changes, running `projen new --from <module>` will not work if the module was compiled with a version of projen before 0.37.x AND you are using a version of projen CLI that is 0.37.x or later. It is still possible to create these projects if you use an older version of projen: `npx projen@0.36.5 --from <module>`.

----

I've had to comment out several tests involving external modules due to these breaking changes. I did look for a while to see if there's some way to try redirecting some of these project types so you could still import older project types, but I could not see a way to do this. If a new projenrc file instantiates a `MyCustomProject` where the original library says `MyCustomProject` extends `projen.TypeScriptProject`, I can't see any way to work around this if we are using the newest projen version. (I could update the integration tests to pass e.g. `--projen-version=0.36.5`, but that would defeat the tests' purpose I think?)

To prevent these kinds of dependency cycles from reoccurring, I've added an integration test workflow that checks it is possible to synthesize a Python base project in python using the generated python package. This can be removed once JSII support has been added for submodule cycle checking (aws/jsii#2511).

----

BREAKING CHANGE: All project class types are now within submodules. `projen new --from` may fail if project is not compatible with the version of the CLI you are using. Full list of type changes:
- All classes and types within the `git`, `json`, and `tasks` submodules can now be imported directly from the root.
- The class `AwsCdkConstructLibrary` is now `awscdk.AwsCdkConstructLibrary`
- The class `AwsCdkTypeScriptApp` is now `awscdk.AwsCdkTypeScriptApp`
- The class `Cdk8sTypeScriptApp` is now `cdk8s.Cdk8sTypeScriptApp`
- The class `ConstructLibrary` is now `cdk.ConstructLibrary`
- The class `ConstructLibraryAws` is now `awscdk.ConstructLibraryAws`
- The class `ConstructLibraryCdk8s` is now `cdk8s.ConstructLibraryCdk8s`
- The class `ConstructLibraryCdktf` is now `cdktf.ConstructLibraryCdktf`
- The class `Eslint` is now `javascript.Eslint`
- The class `GitHubProject` is now `github.GitHubProject`
- The class `Jest` is now `javascript.Jest`
- The class `JsiiProject` is now `cdk.JsiiProject`
- The class `NodePackage` is now `javascript.NodePackage`
- The class `NodeProject` is now `javascript.NodeProject`
- The class `TypeScriptAppProject` is now `typescript.TypeScriptAppProject`
- The class `TypeScriptLibraryProject` is now `typescript.TypeScriptLibraryProject`
- The class `TypeScriptProject` is now `typescript.TypeScriptProject`
- The class `TypescriptConfig` is now `javascript.TypescriptConfig` (!)
- The class `UpgradeDependencies` is now `javascript.UpgradeDependencies`
- The class `UpgradeDependenciesSchedule` is now `javascript.UpgradeDependenciesSchedule`
- The interface `AwsCdkConstructLibraryOptions` is now `awscdk.AwsCdkConstructLibraryOptions`
- The interface `AwsCdkTypeScriptAppOptions` is now `awscdk.AwsCdkTypeScriptAppOptions`
- The interface `Catalog` is now `cdk.Catalog`
- The interface `Cdk8sTypeScriptAppOptions` is now `cdk8s.Cdk8sTypeScriptAppOptions`
- The interface `CodeArtifactOptions` is now `javascript.CodeArtifactOptions`
- The interface `ConstructLibraryAwsOptions` is now `awscdk.ConstructLibraryAwsOptions`
- The interface `ConstructLibraryCdk8sOptions` is now `cdk8s.ConstructLibraryCdk8sOptions`
- The interface `ConstructLibraryCdktfOptions` is now `cdktf.ConstructLibraryCdktfOptions`
- The interface `ConstructLibraryOptions` is now `cdk.ConstructLibraryOptions`
- The interface `CoverageThreshold` is now `javascript.CoverageThreshold`
- The interface `EslintOptions` is now `javascript.EslintOptions`
- The interface `EslintOverride` is now `javascript.EslintOverride`
- The interface `GitHubProjectOptions` is now `github.GitHubProjectOptions`
- The interface `HasteConfig` is now `javascript.HasteConfig`
- The interface `JestConfigOptions` is now `javascript.JestConfigOptions`
- The interface `JestOptions` is now `javascript.JestOptions`
- The interface `JsiiDotNetTarget` is now `cdk.JsiiDotNetTarget`
- The interface `JsiiGoTarget` is now `cdk.JsiiGoTarget`
- The interface `JsiiJavaTarget` is now `cdk.JsiiJavaTarget`
- The interface `JsiiProjectOptions` is now `cdk.JsiiProjectOptions`
- The interface `JsiiPythonTarget` is now `cdk.JsiiPythonTarget`
- The interface `NodePackageOptions` is now `javascript.NodePackageOptions`
- The interface `NodeProjectOptions` is now `javascript.NodeProjectOptions`
- The interface `NodeWorkflowSteps` is now `javascript.NodeWorkflowSteps`
- The interface `PeerDependencyOptions` is now `javascript.PeerDependencyOptions`
- The interface `TypeScriptCompilerOptions` is now `javascript.TypeScriptCompilerOptions` (!)
- The interface `TypeScriptLibraryProjectOptions` is now `typescript.TypeScriptLibraryProjectOptions`
- The interface `TypeScriptProjectOptions` is now `typescript.TypeScriptProjectOptions`
- The interface `TypescriptConfigOptions` is now `javascript.TypescriptConfigOptions` (!)
- The interface `UpgradeDependenciesOptions` is now `javascript.UpgradeDependenciesOptions`
- The interface `UpgradeDependenciesWorkflowOptions` is now `javascript.UpgradeDependenciesWorkflowOptions`
- The enum `NpmAccess` is now `javascript.NpmAccess`
- The enum `Stability` is now `cdk.Stability`
- The enum `TypeScriptJsxMode` is now `javascript.TypeScriptJsxMode`
- The enum `TypeScriptModuleResolution` is now `javascript.TypeScriptModuleResolution`

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants