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

CSUB-1231: Use --profile=production for all builds & CI jobs #437

Closed
wants to merge 3 commits into from

Conversation

atodorov
Copy link
Contributor

@atodorov atodorov commented Aug 8, 2024

Description of proposed changes


Practical tips for PR review & merge:

  • All GitHub Actions report PASS
  • Newly added code/functions have unit tests
    • Coverage tools report all newly added lines as covered
    • The positive scenario is exercised
    • Negative scenarios are exercised, e.g. assert on all possible errors
    • Assert on events triggered if applicable
    • Assert on changes made to storage if applicable
  • Modified behavior/functions - try to make sure above test items are covered
  • Integration tests are added if applicable/needed

@@ -167,7 +167,7 @@ jobs:
id: srtool_build
uses: chevdor/[email protected]
env:
BUILD_OPTS: "--release ${{ needs.setup.outputs.build_options }}"
BUILD_OPTS: "--profile=production ${{ needs.setup.outputs.build_options }}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BradleyOlson64 as-is this change is not being exercised when a PR is created because it gets triggered by a git tag. IMO if https://github.com/gluwa/creditcoin3/actions/runs/10303559931/job/28519918476?pr=437 is failing this one will fail as well.

@atodorov atodorov changed the title CSUB-1231: Use --profile=production for WASM builds too CSUB-1231: Use --profile=production for all builds & CI jobs Aug 8, 2024
Copy link

github-actions bot commented Aug 8, 2024

For full LLVM coverage report click here!

@atodorov atodorov force-pushed the testing/CSUB-1231-profile-production-wasm-build branch from d75e61a to 4fc3e23 Compare August 8, 2024 14:42
@atodorov atodorov force-pushed the testing/CSUB-1231-profile-production-wasm-build branch from 4fc3e23 to c839063 Compare August 8, 2024 15:39
BradleyOlson64
BradleyOlson64 previously approved these changes Aug 8, 2024
Copy link
Contributor

@BradleyOlson64 BradleyOlson64 left a comment

Choose a reason for hiding this comment

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

These changes look right.

Just to make sure, we're fine with CI runs taking 1.5x longer than before right?

Production builds take quite a bit longer than release builds so I was envisioning only using production for final image builds.

@atodorov
Copy link
Contributor Author

atodorov commented Aug 9, 2024

These changes look right.

Just to make sure, we're fine with CI runs taking 1.5x longer than before right?

Can you be more precise? Which jobs are taking 1.5x because I'm not able to spot anything obvious ?

Production builds take quite a bit longer than release builds so I was envisioning only using production for final image builds.

Multiple integration tests with the --profile=production build are currently failing, https://github.com/gluwa/creditcoin3/actions/runs/10305191630/job/28527968917?pr=437 and IMO the only way to have some confidence in these builds is to actually exercise them before we ship. Otherwise we end up in the situation where we test one target, however we ship a different target which is completely untested - not good IMO.

Correction: the failures were due to ommissions to update some of the test scripts, not cause by the SUT itself. However the second point of above statement still holds true.

otherwise we can't really be certain that the binaries we'll ship
actually work as expected!
@atodorov
Copy link
Contributor Author

atodorov commented Aug 9, 2024

Runtime upgrade testing -> https://github.com/gluwa/creditcoin3/actions/runs/10317027713/job/28566440763 seems to me to be working fine.

The rest of the testing is in this PR which also appears to be looking good. Will port to creditcoin3-next and close this one shortly after!

@atodorov atodorov closed this Aug 12, 2024
@atodorov atodorov deleted the testing/CSUB-1231-profile-production-wasm-build branch August 12, 2024 07:02
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.

2 participants