Skip to content

Conversation

mcbarton
Copy link
Collaborator

@mcbarton mcbarton commented Dec 5, 2024

Description

Please include a summary of changes, motivation and context for this PR.

@anutosh491 The Github page workflow was broken when we stopped using condas emsdk. This PR should fix that.

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

@mcbarton mcbarton requested a review from anutosh491 December 5, 2024 12:33
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.80%. Comparing base (4439505) to head (01fd42c).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
- Coverage   81.10%   80.80%   -0.31%     
==========================================
  Files          19       19              
  Lines         974      974              
  Branches       93       93              
==========================================
- Hits          790      787       -3     
- Misses        184      187       +3     

see 1 file with indirect coverage changes

see 1 file with indirect coverage changes

cd $HOME
git clone https://github.com/emscripten-core/emsdk.git
cd emsdk
./emsdk install ${{ matrix.emsdk_ver }}

- name: Setup emsdk
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be removed ? Feels like duplication to me.

Copy link
Collaborator Author

@mcbarton mcbarton Dec 5, 2024

Choose a reason for hiding this comment

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

@anutosh491 This is not duplicating anything as far as I can see. This is a separate workflow and setups emsdk the same way the main workflow does.

Copy link
Collaborator

@anutosh491 anutosh491 Dec 5, 2024

Choose a reason for hiding this comment

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

Wait I am confused. Looking into your file I see this (probably can expand through view file)

      - name: Setup emsdk
        shell: bash -l {0}
        run: |
          cd $HOME
          git clone https://github.com/emscripten-core/emsdk.git
          cd emsdk
          ./emsdk install  ${{ matrix.emsdk_ver }}

      - name: Setup emsdk
        shell: bash -l {0}
        run: |
          emsdk install ${{matrix.emsdk_ver}}   
          

I am just guessing we don't need to do this twice ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @mcbarton Am I missing something ? Check my comment above

Copy link
Collaborator

@anutosh491 anutosh491 left a comment

Choose a reason for hiding this comment

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

Mostly looks good except my comment.
Let's wait for the ci to be green before merging !

@anutosh491 anutosh491 merged commit d8b8a93 into compiler-research:main Dec 5, 2024
10 checks passed
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.

3 participants