Skip to content

Always rebuild it when building dist#15877

Merged
seanbudd merged 4 commits into
nvaccess:masterfrom
hwf1324:fix-13372
Dec 7, 2023
Merged

Always rebuild it when building dist#15877
seanbudd merged 4 commits into
nvaccess:masterfrom
hwf1324:fix-13372

Conversation

@hwf1324

@hwf1324 hwf1324 commented Dec 4, 2023

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #13372

Summary of the issue:

When rebuilding dist, some code changes were not detected. SCons does not trigger a new build.

Description of user facing changes

None

Description of development approach

dist will always be considered obsolete.

Testing strategy:

Test steps from #13372:

The following steps use scons launcher command but all the same applies to scons dist as well.

  1. Get a fresh copy of NVDA:
> git clone --recursive https://github.com/nvaccess/nvda.git
> ...
> cd nvda
  1. Build launcher:
> scons launcher
... many lines of actual building process omitted
scons: done building targets.
  1. Check that launcher was built successfully:
> output\nvda_snapshot_source-master-6310b86.exe
  1. Adding the following lines in core.py in def main right before app.MainLoop():
import tones
tones.beep(1000, 1000)
  1. Running NVDA from source to make sure beep can be heard:
> runnvda.bat
  1. Rebuild launcher:
> scons launcher
...
scons: `launcher' is up to date.
scons: done building targets.

Known issues with pull request:

It will always rebuild when building dist.

After running NVDA in the dist folder, encountered a Windows error during the rebuild. Speculating that py2exe is unable to replace the occupied file.
This issue, although not caused by this PR, should still be mentioned. See details in #15884.

There is no method found to only replace the modified files when rebuilding py2exe

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@codeofdusk

Copy link
Copy Markdown
Contributor

This is intended behaviour, because scons only rebuilds on new commits.

@hwf1324

hwf1324 commented Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

This is intended behaviour, because scons only rebuilds on new commits.

Yes, but see my comment in #13372 that SCons doesn't detect changes in dependencies that trigger a new build.

@lukaszgo1

Copy link
Copy Markdown
Contributor

Would you be able to summarize (preferably with clear steps to reproduce) what are issues caused by this approach?

@hwf1324

hwf1324 commented Dec 5, 2023

Copy link
Copy Markdown
Contributor Author

Would you be able to summarize (preferably with clear steps to reproduce) what are issues caused by this approach?

Okay, I'll do that tomorrow.

For the case where NVDA in dist is run maybe it's not an issue for this PR.Maybe a new issue should be created.

@lukaszgo1

Copy link
Copy Markdown
Contributor

Since #15884 has not been caused by this PR, I don't think this work should be blocked by it.
Ideally dist would only be rebuild when any file in thesource directory changes, though if this is not easily doable this PR is certainly an improvement.
I'm not sure if there is an use case for building dist independently though. Currently the documentation states:

To make a non-archived binary build (equivalent to an extracted portable archive), type: scons dist

however the created folder does not contain nvda.exe.
Can we consider not allowing building dist and change the default target to launcher?

Comment thread user_docs/en/changes.t2t Outdated

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @hwf1324

@seanbudd seanbudd merged commit 7402acd into nvaccess:master Dec 7, 2023
@nvaccessAuto nvaccessAuto added this to the 2024.1 milestone Dec 7, 2023
@hwf1324 hwf1324 deleted the fix-13372 branch December 7, 2023 00:31
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 this pull request may close these issues.

scons launcher and scons dist don't rebuild

5 participants