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

Build: Misc fixes to unblock 'fluid-build --all' #12400

Merged
merged 7 commits into from
Oct 12, 2022
Merged

Build: Misc fixes to unblock 'fluid-build --all' #12400

merged 7 commits into from
Oct 12, 2022

Conversation

DLehenbauer
Copy link
Contributor

Fixes a few nonconformant NPM scripts and a missing package dependency that was preventing 'fluid-build --all' from succeeding:

  • Removed the '-b' parameter from 'tsc -b'.
    • The '-b' argument confused Fluid-Build when attempting to find the 'tsc' task.
    • 'composite: true' already specified in the tsconfig.json.
  • Removed 'npm run clean' from "build" scripts.
    • Cleaning the build output during build breaks the incremental build heuristics.
  • 'version-tools' used 'ts-node' instead of building './test' directory
    • Fluid-Build detected that .eslintrc referenced a *.tsconfig file that wasn't built.
    • Converted 'version-tools' to conform to the './src/test' pattern used by other projects.
  • Adding missing "copyfiles" dependency

After these changes, I could successfully build with:

git clean -xfd
npm i
fb --all --install
fb --all

@DLehenbauer DLehenbauer requested review from msfluid-bot and a team as code owners October 11, 2022 19:52
@github-actions github-actions bot added the base: main PRs targeted against main branch label Oct 11, 2022
@@ -23,8 +23,7 @@
],
"scripts": {
"build": "npm run build:genver && concurrently npm:build:compile npm:lint && npm run build:docs",
"build:clean": "npm run clean",
"build:compile": "npm run build:clean && npm run tsc && npm run build:copy",
"build:compile": "npm run tsc && npm run build:copy",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including "clean" in the compile steps breaks the incremental build heuristics.

Typically manifests as API-Extractor not being able to locate 'index.d.ts'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a safe change for build-cli. It needs to remove some generated files (most notably the manifest) or development gets really weird because the typical dev flow recompiles stuff on the fly -- but not the manifest. So if it's there, commands can appear to go missing etc. because the code and manifest are outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting just the manifest.json file should be okay.

I believe the problem was that at the beginning of the build, 'fluid-build' would determine that 'tsc' could be skipped, but still execute the other tasks from the 'npm:build' script (which would delete the output of TSC).

@@ -1,6 +1,5 @@
{
"require": ["test/helpers/init.js", "ts-node/register"],
"watch-extensions": ["ts"],
"require": ["src/test/helpers/init.js"],
Copy link
Contributor Author

@DLehenbauer DLehenbauer Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fluid-Build noticed that 'src/tests/tsconfig.json' was not built by "npm run build".

I changed the project to compile tests and use the built output rather than invoke 'ts-node'.

Following the pattern used by our other projects, './test' was moved to './src/test'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a reason I couldn't put the tests in /src... but I can't remember what it was. :( It's possible that this change will break making local changes then running /bin/dev without recompiling?

Copy link
Contributor Author

@DLehenbauer DLehenbauer Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried cleaning the directory and running ./bin/dev. 'ts-node' seems happy.

Perhaps in the past the root 'tsconfig.json' file did not exclude '/src/test'?

@tylerbutler
Copy link
Member

Will review more later, but are these warnings that fluid-build spits out or actual build failures? I thought I excluded build-tools from the fluid-build script policies because some shouldn't apply, and people keep complaining about seeing warnings.

@tylerbutler
Copy link
Member

The more I think about it, the more I want fluid-build to know nothing of build-tools. I added it to make it easier to test the release group logic, but with the azure release group there's another place to test stuff outside client. Increasingly I want to be able to go "off the beaten path" with build-tools (for example, using pnpm workspaces until our full-repo infra upgrade is done) and fluid-build makes that difficult.

@DLehenbauer
Copy link
Contributor Author

Are these warnings that fluid-build spits out or actual build failures?

Unfortunately, these are failures that are impacting our external contributors (here).

(Except for removing the unnecessary eslint suppressions... that was just low hanging fruit.)

Copy link
Member

@tylerbutler tylerbutler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go ahead and merge; I'll follow up with other changes.

Copy link
Member

@tylerbutler tylerbutler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I've made a temporary fix in #12415. Please take a look and let me know what you think.

@DLehenbauer
Copy link
Contributor Author

@tylerbutler - Have a look at my latest iteration which preserves your desired workflow. If you're still unconvinced, let's talk.

Copy link
Member

@tylerbutler tylerbutler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Can you make the same change in the version-tools package? It also uses oclif so has the manifest problem.

@DLehenbauer
Copy link
Contributor Author

Can you make the same change in the version-tools package?

Sorry I missed that. Fixed and verified that '/bin/dev' works there as well.

@DLehenbauer DLehenbauer merged commit 25289ab into main Oct 12, 2022
@DLehenbauer DLehenbauer deleted the build-all branch October 12, 2022 18:34
tylerbutler added a commit to tylerbutler/FluidFramework that referenced this pull request Oct 12, 2022
tylerbutler added a commit to tylerbutler/FluidFramework that referenced this pull request Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants