Add dependencies for dependencies that need them
Details
- Reviewers
emack Fabien - Group Reviewers
Restricted Project - Commits
- rABCe857c373ec73: [CI] Use recursive deps in CI builds
@bot ecash-herald-tests token-server-tests mock-chronik-client-tests b58-ts-tests chronik-client-tests ecash-lib-tests ecash-agora-tests ecashaddrjs-tests ecash-script-tests ecash-coinselect-tests ecash-agora-integration-tests ecash-lib-integration-tests chronik-client-integration-tests
Also test the dep builds individually
@bot b58-ts ecash-lib ecash-agora chronik-client mock-chronik-client ecash-script ecash-coinselect ecashaddrjs
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- use-recursive-deps
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 31817 Build 63127: Build Diff Build 63126: arc lint + arc unit
Event Timeline
@bot ecash-herald-tests token-server-tests mock-chronik-client-tests b58-ts-tests chronik-client-tests ecash-lib-tests ecash-agora-tests ecashaddrjs-tests ecash-script-tests ecash-coinselect-tests ecash-agora-integration-tests ecash-lib-integration-tests chronik-client-integration-tests
More a question than actually requesting change.
Also I think you test plan should include running the dependencies directly, and not only the callers.
contrib/teamcity/build-configurations.yml | ||
---|---|---|
689 ↗ | (On Diff #51826) | I'm not sure I understand what you're doing here. Does mock-chronik-client directly depend on ecashaddrjs ? I think not, and since it's already pulled via chronik-client this should be removed ? |
795 ↗ | (On Diff #51826) | e.g. here I would expet ecash-agora, ecash-script and mock-chronik-client only |
contrib/teamcity/build-configurations.yml | ||
---|---|---|
689 ↗ | (On Diff #51826) | As far as CI is concerned -- yes, we could shorten this depends key here to just include chronik-client, since this means ecashaddrjs would be built and then mock-chronik-client would work. Here are the dependencies for mock-chronik-client: "dependencies": { "chronik-client": "file:../chronik-client", "ecashaddrjs": "file:../ecashaddrjs", "ws": "^8.18.0" }, These are the npm packages that are used directly in the index.ts of the app, i.e. stuff that any user of mock-chronik-client needs installed in order for mock-chronik-client to function. The way npm deps work, if you npm i in the mock-chronik-client dir, npm "knows" that ecashaddrjs is already a dep of chronik-client, so you end up installing it only one time. In general, a dev isn't expected to "know" what all his deps depend on. This is how we can often get into "dependency hell" ... an app dev may really want a certain version of ecashaddrjs, but some other dep he users wants another version, probably because it has not been updated ... and now we have a problem. Our CI does not currently "know" deps of deps, so it will install it twice if we list it twice. Options 1 - We leave the depends keys here as they are, this is simpler as they exactly match what the app actually depends on. However it means we are double building some apps. We could fix this by adding checks so that we only npm ci && npm run build on an app if this has not already been done by another step; i.e. we check for a dist/ folder or a node_modules folder. Or we could just allow double installs to happen. 2 - We could also change nothing in CI and just be more diligent about how we define what goes into depends, i.e. in this case we could simply remove ecashaddrjs since we "know" it will be pulled in by the build of chronik-client I think option 1 is best here, since it mimics how this works with npm, and would be the easiest for someone who is not deeply familiar with the CI to add another service. The "no double install" check is simple enough to implement. This is still doing more than minimal work for CI ... so there is an argument to be made for option 2 here. It's not like we add new apps / plans to this every day, and having 1 or 2 staff devs who "get it" is probably enough to prevent any issues. What do you think? |
795 ↗ | (On Diff #51826) | see comment above. indeed, it would be possible to do it this way. The trade-off is each app needs to go through a mental translation process from "what is in its dependencies array in its own folder" and "what is the minimal way to make sure all of those are built" this stuff does not change often and any dep change that breaks CI would be caught in the introducing diff |
@bot b58-ts ecash-lib ecash-agora chronik-client mock-chronik-client ecash-script ecash-coinselect ecashaddrjs
contrib/teamcity/build-configurations.yml | ||
---|---|---|
689 ↗ | (On Diff #51826) | Solution 1 is fine if that matches the package.json. Also I don't think npm will do much on second call since it's the same build directory every time. |