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.

Sign up for GitHub

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

Jump to bottom

Diagram bug fixed #116

Merged
merged 6 commits into from
May 17, 2023
Merged

Diagram bug fixed #116

merged 6 commits into from
May 17, 2023

Conversation

petervdonovan
Copy link
Contributor

This partially reverts commit f85abfc.

This fixes #113.

Thank you to @soerendomroes for helping us find the source of the problem.

@petervdonovan petervdonovan requested a review from lhstrh May 17, 2023 01:41
@lhstrh
Copy link
Member

lhstrh commented May 17, 2023

Looks like the fix introduced another problem...

@petervdonovan
Revert "update build"
60a31cd 
This reverts commit f85abfc.

It was also necessary to update node in order to get
`npx tsc --lib "dom" --outDir out/test --inlineSourceMap`
to succeed without errors.
@petervdonovan
Rename syntax -> syntaxes.
594532d
@petervdonovan
Increase maxInstallationTimeMilliseconds.
2f2aa3d 
This is a response to very flaky PMPM installation tests on Windows
(test-dependencies-missing-extended).
@petervdonovan
Double times given to tests (again)
3ab9edc 
This is in response to persistently failing
test-dependencies-missing-extended tests on Windows, in particular for
PNPM.
@petervdonovan
Update known good to match lingua-franca submodule
911e413
@petervdonovan
Attempt to address Windows test failure.
7ede3c3 
I believe that this results from the caching improvements that were
recently made specifically for Windows.
lhstrh
lhstrh reviewed May 17, 2023
View reviewed changes
.github/actions/build/action.yml
Copy link
Member

Choose a reason for hiding this comment

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

Why did this have to be changed? We use prepare-build-env almost everywhere, so it should just work...

lhstrh
lhstrh reviewed May 17, 2023
View reviewed changes
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

What's the nature of the changes in the known-good texts?

@petervdonovan
Copy link
Contributor Author

Why did this have to be changed? We use prepare-build-env almost everywhere, so it should just work...

There is an error due to the CWD being vscode-lingua-franca instead of lingua-franca during the cleanup process at the end. See this test failure (in main) for example.

What's the nature of the changes in the known-good texts?

I updated the lingua-franca submodule, so the content of the tests changed.

@lhstrh
Copy link
Member

lhstrh commented May 17, 2023

Why did this have to be changed? We use prepare-build-env almost everywhere, so it should just work...

There is an error due to the CWD being vscode-lingua-franca instead of lingua-franca during the cleanup process at the end. See this test failure (in main) for example.

I see. It's a bit mysterious as to why this error cropped up all of a sudden, but the workaround is fine with me.

What's the nature of the changes in the known-good texts?

I updated the lingua-franca submodule, so the content of the tests changed.

👍

@petervdonovan
Copy link
Contributor Author

Probably the Windows error was related to the caching improvements. It's the only recent change that I was aware of.

@petervdonovan petervdonovan merged commit 36fac9c into main May 17, 2023
10 checks passed
@petervdonovan petervdonovan deleted the fix-invalid-arguments branch May 17, 2023 19:38
@lhstrh lhstrh added the bugfix label Aug 28, 2023
@lhstrh lhstrh changed the title Partially revert "update build" Diagram bug fixed Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhstrh lhstrh lhstrh approved these changes

Assignees
No one assigned
Labels
bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Diagrams bug
2 participants
@petervdonovan @lhstrh