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

#878: default version from mirrors #893: version prefix #940: generic doInstall #934

Merged

Conversation

hohwille
Copy link
Member

@hohwille hohwille commented Oct 10, 2022

Implements the following stories and features:

Revealed bug #933 that is now fixed.

@hohwille hohwille added this to the release:2022.08.004 milestone Oct 10, 2022
@hohwille hohwille linked an issue Oct 10, 2022 that may be closed by this pull request
@github-actions github-actions bot added bash related to bash shell or scripts commandlet related to commandlets (scripts/command/*) documentation related to documentation (AsciiDoc) scripts related to shell scripts (bash and CMD) labels Oct 10, 2022
@hohwille hohwille mentioned this pull request Oct 11, 2022
hohwille added a commit to hohwille/ide that referenced this pull request Oct 11, 2022
@github-actions github-actions bot removed the documentation related to documentation (AsciiDoc) label Oct 11, 2022
@github-actions github-actions bot added the documentation related to documentation (AsciiDoc) label Oct 14, 2022
@hohwille
Copy link
Member Author

hohwille commented Oct 14, 2022

This is now working and fine.
However, this is a rather significant change that could easily break things.
Therefore, it would be good to do the following:

  • do a review
  • do manual (and automated) tests of each commandlet to verify that nothing broke
  • explicitly test with a custom tool to see that this functionality is still working.

Besides I am considering more changes.
I already refactored doInstall significantly (number and order of parameters).
I could do the same also with doDownload for further cleanup and simplification but this is rather optional.
I am tempted to also implement #940 and/or #893 in this PR as those stories are very closely related.
However, we could still do it in a separate PR after merging this one...

@hohwille hohwille linked an issue Oct 14, 2022 that may be closed by this pull request
@hohwille
Copy link
Member Author

#893 was now easy to add so I just implemented it :)
Works like a charm

@tobka777 tobka777 linked an issue Oct 27, 2022 that may be closed by this pull request
@cinnamon-coder-hub
Copy link
Member

cinnamon-coder-hub commented Oct 31, 2022

As some of the problems mentioned before may be related to Linux or just have to be settled at some point,
I could tackle them, or a subset, after the issue #941 progressed enough. Also, as I mentioned in our daily, those issues are not necessarily related to the code changes of this PR, but came up during testing for it.
Maybe fixing the above problems would even be advantageous for writing methods handling the differing download urls for the issue #941.

alfeilex and others added 3 commits November 1, 2022 10:21
There was in an error installing pip on Windows because the installation file was renamed from pip-latest-windows-py to pip-latest-pip.py
@alfeilex
Copy link
Member

alfeilex commented Nov 1, 2022

On MacOS, Python is not working. See here:

Warning: openssl@3 3.0.5 is already installed and up-to-date.
To reinstall 3.0.5, run:
  brew reinstall openssl@3
Success: run command brew
sh: /Users/m1/Documents/Alex_dev/test_doInstall/workspaces/main/test/scripts/target/integration-test/test-setup/software/python/configure: No such file or directory
make: *** No targets specified and no makefile found.  Stop.
make: *** No rule to make target `altinstall'.  Stop.
make: *** No rule to make target `clean'.  Stop.
make: *** No rule to make target `distclean'.  Stop.
m1@3dc6a48e-5e10-4e97-a481-1c8483f2748d test-setup % which python
/usr/bin/python
m1@3dc6a48e-5e10-4e97-a481-1c8483f2748d test-setup % where python
/usr/bin/python
m1@3dc6a48e-5e10-4e97-a481-1c8483f2748d test-setup % devon python
Version 3.10.8 of python is already installed.
Python 2.7.18
Success: verify installation of python (python)

When Python is installed globally, devon python appears to run the global Python executable.

Full log can be found here: https://github.com/alfeilex/ide/actions/runs/3368406824/jobs/5586914231#step:5:831

@alfeilex
Copy link
Member

alfeilex commented Nov 1, 2022

More MacOS errors related to this MacOS workaround:

doEcho "Creating symlink as workaround for ${software} on MacOS"
local target_software_macos=${target_path%\/*}/macos
if [ ! -d "${target_software_macos}" ]
then
mkdir -p "${target_software_macos}"
fi
local target_software_software=${target_software_macos}/${dir}
doRunCommand "rm -rf '${target_software_software}'"
doRunCommand "mv '${target_path}' '${target_software_software}'"
doRunCommand "ln -sf 'macos/${dir}/${contents}/${app_folder}' '${target_path}'"
doRunCommand "cp '${target_software_software}/.devon.software.version' '${target_path}'"

Possible error
This workaround also runs for commands like terraform, oc or dotnet and probably should not be run. I was not able to fix this error. This is the error output:

Creating symlink as workaround for terraform on MacOS
Success: run command rm
Success: run command mv
Success: run command ln
cp: /Users/m1/Documents/Alex_Dev/test_doInstall/workspaces/main/test/scripts/target/integration-test/test-setup/software/terraform/.devon.software.version and /Users/m1/Documents/Alex_Dev/test_doInstall/workspaces/main/test/scripts/target/integration-test/test-setup/software/macos/terraform/.devon.software.version are identical (not copied).

Full log: https://github.com/alfeilex/ide/actions/runs/3368406824/jobs/5586914231#step:5:841

@hohwille
Copy link
Member Author

hohwille commented Nov 3, 2022

This workaround also runs for commands like terraform, oc or dotnet and probably should not be run. I was not able to fix this error. This is the error output:

This is what we already started to discuss. What we need is a proper analysis why this happens.
I reproduced the bug on my Mac and found out this:

# ls -la software/terraform
... software/terraform -> macos/terraform//
# ls -la software/terraform/terraform
-rwx... software/terraform/terraform

So there is a bug in the MacOS Workaround detection. Terraform is not a MacOS Application with a Content folder.
However, it is falsely detected as such and therefore created inside software/macos/terraform and the symlink is pointing to that folder itself what should never happen as it never makes sense (in that case we could directly extract it to software/terraform).

@hohwille
Copy link
Member Author

hohwille commented Nov 3, 2022

So there is a bug in the MacOS Workaround detection.

I found and fixed it. The function doGetFirstExistingPath does nothing if no existing path found was found. Hence ${contents} is empty but this was not checked and obviously [ -d "${target_path}/${contents}" ] applies then.
Problem resolved. Thanks for finding this!

@hohwille
Copy link
Member Author

hohwille commented Nov 3, 2022

BTW: Starting from here I stop accepting more errors in this PR that are not related to my changes at all. Otherwise, if we try to find and fix all bugs of the last two years, we will never be able to finish this "monster pr". Do not get me wrong: Your feedback is extremely valuable and you found important bugs I made in this PR. However, we need to focus on getting this PR merged and with #963 fixed so that we can reach a working state. It is fine and more than welcome to file to file more issues for bugs we might have in devonfw-ide but if they are not related to the changes of this PR, we should keep them out of here.

@hohwille
Copy link
Member Author

hohwille commented Nov 4, 2022

I tested this PR again with the latest code and after #963 being fixed.
Now everything looks good to me except python (what is most probably not related to this PR at all):

Successfully installed python
The software python has been added. You need to rerun 'devon' command without arguments or restart your terminal to update your PATH so the newly installed software will be found.
Freeing Python from embedded Mode...
Traceback (most recent call last):
  File "runpy.py", line 196, in _run_module_as_main
  File "runpy.py", line 86, in _run_code
  File "D:\projects\devonfw\software\pip\bin\pip.exe\__main__.py", line 4, in <module>
ModuleNotFoundError: No module named 'pip'

******** ATTENTION ********
Failed to pip install azure-cli (pip)
We are sorry for the inconvenience. Please check the above errors, resolve them and try again.
Exit code was 1

@hohwille hohwille merged commit 478794d into devonfw:master Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bash related to bash shell or scripts commandlet related to commandlets (scripts/command/*) documentation related to documentation (AsciiDoc) scripts related to shell scripts (bash and CMD) test related to testing and QA
Projects
None yet
6 participants