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: only default to -U if there is not a version in the config [APE-1180] #26

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

antazoey
Copy link
Member

@antazoey antazoey commented Jul 8, 2023

(finishes) fixes: #16

Before, if you didn't specify any ape-plugins-list input, it would always try -U ..
However this causes issues when the ape-config.yaml file contains version specifications.
Thus, to fix, we detect if there are versions specified in the yaml. If there are, default to only .. Else, default to -U . as we did before.

So this is not a breaking change because it will still work as it did except in the times when it didn't work at all, now it will work then too, so it is just a bugfix

As a plus, I added tests
. Look at the CI!

@vany365 vany365 changed the title fix: only default to -U if there is not versions in the config fix: only default to -U if there is not versions in the config [APE-1180] Jul 8, 2023
@antazoey antazoey changed the title fix: only default to -U if there is not versions in the config [APE-1180] fix: only default to -U if there is not a version in the config [APE-1180] Jul 9, 2023
@@ -16,7 +16,7 @@ inputs:
ape-plugins-list:
description: 'Space seperated list of plugins to install with relevant pinning applied'
required: False
default: '-U .'
default: ''
Copy link
Member

Choose a reason for hiding this comment

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

Should this be just .

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I don't think so anyway.

This is how it works:

If the user is not specifying anything here (using the default), it will calculate a default.
if we make the default ".", it would not be very easy to detect whether the user has specified "." or they are using the default behavior.

@fubuloubu
Copy link
Member

Note: when this is released, update the major version tag and tag it as a new minor too

@antazoey
Copy link
Member Author

Note: when this is released, update the major version tag and tag it as a new minor too

I was trying to avoid the breaking change because the default value will still be used in all cases where it worked.
the only time it would not be used would be in cases where it wasn't working anyway, thus avoiding this breaking something for someone.

@fubuloubu
Copy link
Member

Note: when this is released, update the major version tag and tag it as a new minor too

I was trying to avoid the breaking change because the default value will still be used in all cases where it worked.
the only time it would not be used would be in cases where it wasn't working anyway, thus avoiding this breaking something for someone.

No I mean update the current v3 tag to point at the new commit

@antazoey antazoey merged commit 64421da into ApeWorX:main Aug 31, 2023
7 checks passed
@antazoey antazoey deleted the fix/plugins branch August 31, 2023 12:28
@pcaversaccio
Copy link
Contributor

pcaversaccio commented Sep 2, 2023

hey @antazoey @fubuloubu, I'm not 100% sure but I think this PR broke my CI pipeline: https://github.com/pcaversaccio/snekmate/actions/runs/6057836515/job/16439414869#step:6:1219

My ape-config is:

name: snekmate
contracts_folder: src
default_ecosystem: ethereum
plugins:
  - name: vyper
vyper:
  evm_version: shanghai

and the CI setup:

- name: Setup Ape
  uses: ApeWorX/github-action@v2
  with:
    python-version: ${{ matrix.python-version }}

- name: Check Ape compilation
  run: ape compile

Could you maybe have a look at this quickly since it's pretty important for snekmate as it's part of the test smart contracts pipeline and I need to push some updates that won't get tested currently due to this failure.

@fubuloubu
Copy link
Member

I reverted the v2 tag before this merged PR since it seems to have an issue in the specific configuration that @pcaversaccio is experiencing

@pcaversaccio
Copy link
Contributor

Thanks a lot @fubuloubu. The bash script seems to throw somehow:

Run if [[ "true"  == "true" ]]; then
  if [[ "true"  == "true" ]]; then
    version_present=$(sed -n '/^plugins:/,/^[^ ]/{/version:/p;}' "ape-config.yaml" | grep -c version)
  else
    version_present=0
  fi
  
  if [[ "${version_present}" == "1" ]] && [[ -z "" ]]; then
    plugins_value="."
  elif [[ -z "" ]]; then
    plugins_value="--upgrade ."
  else
    plugins_value=""
  fi
  
  ape plugins install "$plugins_value"
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.11.4/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.4/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.4/x64
    Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.4/x64
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.4/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.4/x64/lib
##[debug]/usr/bin/bash --noprofile --norc -e -o pipefail /home/runner/work/_temp/7b472a5e-f65c-4e38-9823-f9a532a9bbef.sh
Error: Process completed with exit code 1.

@antazoey
Copy link
Member Author

antazoey commented Sep 2, 2023

It looks like a bug in "version_present" variable not interpolating and being compared as a literally. I'll fix after the weekend.

@antazoey
Copy link
Member Author

antazoey commented Sep 5, 2023

im pretty positive this fixes all the issues: #29
but if you are able to try it out, let me know!

@pcaversaccio
Copy link
Contributor

FWIW, you can simply fork my repo and run the CI test-contracts to test it.

@antazoey
Copy link
Member Author

antazoey commented Sep 5, 2023

FWIW, you can simply fork my repo and run the CI test-contracts to test it.

Ok, I did that here: https://github.com/antazoey/snekmate/actions/runs/6085791567/job/16510699465?pr=1
That run used the changes and the whole action passed.
Thanks

@pcaversaccio
Copy link
Contributor

Awesome, thanks for the heads-up!

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.

plugins list causes workflow to fail
3 participants