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

fix(build): support running Blocky locally on Windows machines #7281

Merged
merged 7 commits into from
Aug 2, 2023
Merged

fix(build): support running Blocky locally on Windows machines #7281

merged 7 commits into from
Aug 2, 2023

Conversation

abdul-alhasany
Copy link
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

#6864 and #7097

Fixes

Proposed Changes

Make building process compatible with machines run on windows.

Behavior Before Change

When running npm run start errors occur as described in the bug reports above.

Behavior After Change

Running npm run start is working with no errors on Windows.

Reason for Changes

Help developers who work on Windows machines to run blocky locally and potentially contribute to the codebase.

Test Coverage

I did not create any tests. I tested the code on my Windows machine.

Documentation

I don't think any further documentation required for this change. Current documentation already asks users to run npm run start. The user does not need to do anything specific on Windows machines.

Additional Information

No

When using single quote on windows, e.g. 'build/src', the folder
are created with a single quote at the beginning `'build` and end
`src'`. This commit fixes this issue.
Ensure that when running on windows, python command is python and
not python3. Also, separators are normalized to posix style `/` even on
windows system
@google-cla
Copy link

google-cla bot commented Jul 12, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@abdul-alhasany abdul-alhasany changed the title Support running Blocky locally on Windows machines fix: support running Blocky locally on Windows machines Jul 12, 2023
@github-actions github-actions bot added the PR: fix Fixes a bug label Jul 12, 2023
@abdul-alhasany abdul-alhasany changed the title fix: support running Blocky locally on Windows machines fix(build): support running Blocky locally on Windows machines Jul 12, 2023
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 12, 2023
@rachel-fenichel
Copy link
Collaborator

This looks right to me and tests pass. @cpcallen can you double-check the chunking command in the last file?

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Hi @abdul-alhasany! Thanks for this pull request. We especially appreciate external developers who use Windows helping us with issues like this because we've not had any frequent Windows users on the core Blockly team for some time. I'm only sorry that I didn't have time to look at it until now.

Do have a look at my comments, questions and suggested changes below, and let me know if there is anything that is not clear.

scripts/gulpfiles/build_tasks.js Outdated Show resolved Hide resolved
scripts/gulpfiles/build_tasks.js Outdated Show resolved Hide resolved
Comment on lines 566 to 569
chunk => chunkFiles.find(f => {
return f.endsWith(path.sep.replace("\\", "/") + chunk.entry.replaceAll("\\", "/"));
}
));
Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm unclear about—and what would be very helpful if you could give us some information about—is what the strings in the chunkFiles array look like.

On macOS (and Linux etc.) they use / as a separator, and I was expecting that they would use \ on win32, and so using f.endsWith(path.sep + chunk.entry) (where chunk.entry also uses path.sep) should work fine.

But from this proposed code change it seems that for chunkFiles uses / as a path separator even on Windows. Is that correct?

In any case, the proposed change can be simplified somewhat, since path.sep is only ever either '/' or '\\':

Suggested change
chunk => chunkFiles.find(f => {
return f.endsWith(path.sep.replace("\\", "/") + chunk.entry.replaceAll("\\", "/"));
}
));
chunk => chunkFiles.find(f => f.endsWith('/' + chunk.entry.replaceAll("\\", "/")));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows accepts both \ and / which is why, in my projects, I generally opt in for path.replaceAll("\\", "/") to make the behavior consistent across os systems. The reason I also did not replace path.sep is because I did not want to introduce extra changes. I wanted to keep the original code the same as much as possible.

There are all chunk entries that pass the condition (f.endsWith):

build\src\core\main.js
build\src\blocks\blocks.js
build\src\generators\javascript.js
build\src\generators\python.js
build\src\generators\php.js
build\src\generators\lua.js
build\src\generators\dart.js

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Please run npm run format to fix formatting issues.

@abdul-alhasany
Copy link
Contributor Author

build_task.js is ignored by .eslintignore. What formatting issues are there? I can do it manually if it is required.

@cpcallen
Copy link
Contributor

cpcallen commented Aug 1, 2023

build_task.js is ignored by .eslintignore. What formatting issues are there? I can do it manually if it is required.

Our lint checks are configured specifically not to do formatting checks, which are done by a separate GitHub workflow (action). I thought I had seen a format check failure due to the double quotes, but apparently either I was mistaken or the failure was transitory, as I see it is now passing.

Nevertheless, please fix quoting as noted.

@abdul-alhasany
Copy link
Contributor Author

No problem. All done.

@cpcallen cpcallen enabled auto-merge (squash) August 2, 2023 16:24
@cpcallen cpcallen merged commit 5e0390b into google:develop Aug 2, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants