Skip to content

+az (azure-cli)#451

Merged
jhheider merged 4 commits into
pkgxdev:mainfrom
mfts:azure-cli
Mar 5, 2023
Merged

+az (azure-cli)#451
jhheider merged 4 commits into
pkgxdev:mainfrom
mfts:azure-cli

Conversation

@mfts
Copy link
Copy Markdown
Contributor

@mfts mfts commented Mar 3, 2023

closes #302

@what-the-diff
Copy link
Copy Markdown

what-the-diff Bot commented Mar 3, 2023

  • The package.yml file was added to the project
  • It contains a list of dependencies, build instructions and test commands for azure-cli
  • A new version is created from github tags by stripping off "azure-cli-" prefixes in each tag name
  • Build script creates a virtual environment with python 3 installed and installs all required packages into it (including itself) using pip install command
  • Test script runs an example command that shows information about AzureCloud cloud provider

@mfts
Copy link
Copy Markdown
Contributor Author

mfts commented Mar 3, 2023

The test is actually not working properly. It works locally on macOS. I guess it's something with the virtualenv. Will continue to check

Comment on lines +24 to +49
script: |
python -m venv {{prefix}}/venv
cd {{prefix}}/venv
pip install {{prefix}}/../src-v{{version}}/src/azure-cli
pip install {{prefix}}/../src-v{{version}}/src/azure-cli-core
pip install {{prefix}}/../src-v{{version}}/src/azure-cli-telemetry
mkdir -p ../bin

cat <<EOF > ../bin/"az"
#!/bin/sh
export VIRTUAL_ENV="\$(cd "\$(dirname "\$0")"/.. && pwd)/venv"
TEA_PYTHON="\$(which python)"
TEA_PYHOME="\$(dirname "\$TEA_PYTHON")"
cat <<EOSH > \$VIRTUAL_ENV/pyvenv.cfg
home = \$TEA_PYHOME
include-system-site-packages = false
executable = \$TEA_PYTHON
EOSH
find "\$VIRTUAL_ENV"/bin -maxdepth 1 -type f | xargs \\
sed -i.bak "1s|.*|#!\$VIRTUAL_ENV/bin/python|"
rm "\$VIRTUAL_ENV"/bin/*.bak
ln -sf "\$TEA_PYTHON" "\$VIRTUAL_ENV"/bin/python
exec "\$VIRTUAL_ENV"/bin/az "\$@"
EOF

chmod +x ../bin/"az"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does our existing python-venv.sh script not work for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no because in the root of the repo there's no setup.py. azure has three subrepos (see line 27-29)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It works locally, but I feel like something's not right here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm. What if you ran them back-to-front:

for venv in azure-cli{-telemetry,-core,}; do
  python-venv.sh "{{prefix}}"/az
done

?

Just thinking that gets the most battle-testing in terms of venv management.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Doesn't work like that unfortunately, or I don't understand where you'd like to add it. We need some things that python-venv.sh is doing but not all

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Roger. I figure that'd build all three parts into the venv, and overwrite the final linkage each time; but if it's doesn't work, it doesn't work.

Copy link
Copy Markdown
Contributor Author

@mfts mfts Mar 3, 2023

Choose a reason for hiding this comment

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

Ok I may have found a way:

for venv in azure-cli{-telemetry,-core,}; do
  SRCROOT={{prefix}}/../src-v{{version}}/src/"$venv" python-venv.sh "{{prefix}}"/bin/az
done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Magical. I love standardization.

@mfts
Copy link
Copy Markdown
Contributor Author

mfts commented Mar 3, 2023

Cool it worked and tests pass too

freedesktop.org/pkg-config: '*'
script: |
for venv in azure-cli{-telemetry,-core,}; do
SRCROOT={{prefix}}/../src-v{{version}}/src/"$venv" python-venv.sh "{{prefix}}"/bin/az
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think {{prefix}}/../src-v{{version}}/src/"$venv" is the same thing as ./src/"$venv".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

throws an error if i do that

Error: invalid absolute path: ./src/azure-cli-telemetry
    at new Path (file:///opt/tea.xyz/src-v0.24.9/src/vendor/Path.ts:37:19)
    at from_env (file:///opt/tea.xyz/src-v0.24.9/src/app.ts:175:22)
    at injection (file:///opt/tea.xyz/src-v0.24.9/src/app.ts:165:16)
    at file:///opt/tea.xyz/src-v0.24.9/src/app.ts:34:36

Copy link
Copy Markdown
Contributor

@jhheider jhheider Mar 4, 2023

Choose a reason for hiding this comment

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

Hm. Guess that only works for TEA_PANTRY_PATH. Other packages (like tea.xyz/gx/cc) suggest that $SRCROOT might be set by default. I don't know if this better:

_SRCROOT="$SRCROOT"
for ......
   SRCROOT="$_SRCROOT"/src/"$venv" ...
...

That's not much of an improvement, but it protects against future changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't really understand this _SRCROOT="$SRCROOT" but it works :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ha. Captures the original $SRCROOT before we start overwriting it.

@jhheider jhheider merged commit 7506d01 into pkgxdev:main Mar 5, 2023
This was referenced Mar 7, 2023
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.

+azure-cli (490/548)

2 participants