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

Assorted apio blocking items #453

Closed
zapta opened this issue Nov 12, 2024 · 31 comments
Closed

Assorted apio blocking items #453

zapta opened this issue Nov 12, 2024 · 31 comments

Comments

@zapta
Copy link
Collaborator

zapta commented Nov 12, 2024

Hi @cavearr, I am nearing the end of the planned feature list and next I will focus on testing and cleanup. Here are a few action items for you that blocks the cleanup.

  1. [DONE] Build oss-cad-tools release 0.2.0 so I can switch the apio dev to it (should not affect apio prod)

  2. Make icestudio using apio.ini so we can make it required and do a bunch of related clean ups.

  3. If icestudio uses apio time please switch to apio report so we can delete apio time.

  4. if icestudio uses apio install or apio uninstall, switch to apio packages so we can delete them.

  5. If icestudio uses apio verify switch to apio lint so we can delete it.

If you have any questions please let me know.

@Obijuan
Copy link
Member

Obijuan commented Nov 12, 2024

  1. Build oss-cad-tools release 0.2.0 so I can switch the apio dev to it (should not affect apio prod)

I've tried to build it but the scripts gives an error:

https://github.com/FPGAwars/tools-oss-cad-suite/actions/runs/11792202730/job/32845461579

When creating the window package it is run out of space: "No space left on device"
Not sure if it is a temporal error or not. I will try to build it later. If the error persist, I will create a release and we can upload the packages manually

@zapta Sorry for no answering all your questions. During this quarter I have to give a lot of lectures at the university and I have no time to spend on Apio. But I will have more free time in a month or so. My stutends use Icestudio 0.12 a lot (with apio 0.9.5). Once the lectures are finished, I will test apio and icestudio for the next release cycle (the new version is the one I will use with the students on the next course)

@cavearr
Copy link
Member

cavearr commented Nov 12, 2024

Hi @zapta , I recommend that your students use the latest wips https://downloads.icestudio.io

This week I started uploading many changes that stabilize and fix UI/UX bugs and in a few days it will integrate your celery changes.

I am going to integrate into icestudio this week the new tools that celery will include by default.

Regarding the actions problem, I am already in the process of solving it, actions have a maximum size, if you cover it you have to go to other options (external workers hosted in your infrastructure and things like that) I am going to try to solve it by deleting temporary files during the build.

Thanks for the great work on Apio.

@cavearr
Copy link
Member

cavearr commented Nov 12, 2024

@zapta , @Obijuan 0.2.0 release are ready!!! 💯

@cavearr
Copy link
Member

cavearr commented Nov 12, 2024

I think today at the end of the day i'm relasing a new wip with your changes.

@zapta
Copy link
Collaborator Author

zapta commented Nov 12, 2024

Hi @cavearr, sent you #454 to switch apio dev from my repo to the FPGAwars release you created. After you will update your apio, run that command to update the package and review the repo and version it uses.

@zapta
Copy link
Collaborator Author

zapta commented Nov 19, 2024

Regarding item 2, some of the deprecated stuff we can delete there are the flags --size --type --pack, but it's possible that they don't work anyway and therefore can be deleted now.

See #468 for details.

@zapta
Copy link
Collaborator Author

zapta commented Nov 26, 2024

Hi @cavearr, any luck with testing the latest apio?

I am ready to make apio.ini required and delete the deprecated commands, once you OK it.

@cavearr
Copy link
Member

cavearr commented Nov 26, 2024

Hi @zapta! i'm on it, i need probably this week to finish the tests and the integration because has been more difficult that i expected at the beginning (icestudio part with all improvements i want to close around this).

I'm telling you when i'll done.

Thanks!

@zapta
Copy link
Collaborator Author

zapta commented Dec 7, 2024

Hi @cavearr, what's the status regarding the deprecated commands? Can I go ahead delete them?

Deprecated commands:
  modify     [Depreciated] Modify the apio.ini project file.
  time       [Depreciated] Report design timing.
  verify     [Depreciated] Verify the verilog code.
  install    [Depreciated] Install apio packages.
  uninstall  [Depreciated] Uninstall apio packages.

@cavearr
Copy link
Member

cavearr commented Dec 7, 2024 via email

@cavearr
Copy link
Member

cavearr commented Dec 15, 2024

@zapta i have problems with paths with white space in it (linux and osx, i'll check windows with all works at linux and osx), for example:

"/icestudio/app/resources/collection/examples/1. Basic"
(this is the first example of Icestudio, blink a led) apio execute over:
"/icestudio/app/resources/collection/examples/1. Basic/ice-build/ice-build/01. One LED"

Try to replicate a path with a lot of spaces in diferent folders and try to apply apio commands.

¿Could you check it?

@zapta
Copy link
Collaborator Author

zapta commented Dec 16, 2024

Hi @cavearr, this is a known issue. Iverilog -B has a problem with spaces. Do you know if we need the -B flag?

#474

steveicarus/iverilog#1187

I removed spaces from the paths we use for testing to make the tests pass.

[FUNNY_MARKER = "fuññ

@cavearr
Copy link
Member

cavearr commented Dec 16, 2024

Hi @zapta! i'm playing with it, i'm telling you along the day for advances.

I think could be very useful has a "core" apio flag for debugging, for example:

apio --debug lint

and with that flag we can define debug messages for example for print the commands that apio execute, if you don't like the debug flag, what do you think to manage debug messages with an environment variable, for example APIO_DEBUG, at launch apio if detect this environment variable we could print messages , what do you think?

@cavearr
Copy link
Member

cavearr commented Dec 16, 2024

Hi @zapta ! i don't understand well why before this upgrade apio works well with spaces. Is truth that i'm trying the tools directly (iverilog....) and doesn't work with spaces in the path. Very strange but it is.

While this not work in the original tools, we can't support spaces in the path, i'll try to look for this this afternoon , but if i don't fin any solution, i think i put controls in Icestudio to rename paths with spaces and put this feature in standby because this is a blackhole of time.

@zapta
Copy link
Collaborator Author

zapta commented Dec 16, 2024

Hi @cavearr, I think it's a good idea to have a check in IceStudio until we resolve this issue.

Apio also has a similar check.

ApioContext._check_no_spaces_in_dir(self.project_dir, "project")

@cavearr
Copy link
Member

cavearr commented Dec 16, 2024

yes! just now i'm playing with it, i think i'm near a solution but for now not works yet.

@zapta
Copy link
Collaborator Author

zapta commented Dec 16, 2024

Hi @cavearr, the debug flag or var is a good idea and I can implement it, but I prefer to do that after I clean apio.

Any news on items 2-5? Can I delete the deprecated commands (already have a pending change) and make apio.ini required?

@zapta
Copy link
Collaborator Author

zapta commented Dec 16, 2024

@cavearr, for testing your whitespace solution try to run the apio tests make check after you make these changes

  1. Add a space before and after the 'funny' here

    FUNNY_MARKER = "fuññy"

  2. Comment out the code of this function

    if re.search("\\s", str(dir_path)):

@cavearr
Copy link
Member

cavearr commented Dec 16, 2024

@zapta, If only affect to apio develop go ahead! i'm working in icestudio wips and some things not work now for the latest changes in apio, for this go ahead and i still working in stabilize the wip as soon as possible.

Thanks for the tip!

@zapta
Copy link
Collaborator Author

zapta commented Dec 16, 2024

Thanks @cavearr. Yes, it's in the develope branch. I will got ahead and send the changes, deleting the 5 deprecated commands and making apio.ini required and only source of project configuration.

@cavearr
Copy link
Member

cavearr commented Dec 16, 2024

2. apio/apio_context.py

Hi @zapta doing this , all appears works well? a could try in other way, but i want to use it if you make this test to check the whitespates, but the output is all ok:

python -m tox --skip-missing-interpreters false -e py312 -- --offline
.pkg: _optional_hooks> python /Users/carl/opt/miniconda3/envs/icestudio/lib/python3.13/site-packages/pyproject_api/_backend.py True flit_core.buildapi
.pkg: get_requires_for_build_sdist> python /Users/carl/opt/miniconda3/envs/icestudio/lib/python3.13/site-packages/pyproject_api/_backend.py True flit_core.buildapi
.pkg: get_requires_for_build_wheel> python /Users/carl/opt/miniconda3/envs/icestudio/lib/python3.13/site-packages/pyproject_api/_backend.py True flit_core.buildapi
.pkg: prepare_metadata_for_build_wheel> python /Users/carl/opt/miniconda3/envs/icestudio/lib/python3.13/site-packages/pyproject_api/_backend.py True flit_core.buildapi
.pkg: build_sdist> python /Users/carl/opt/miniconda3/envs/icestudio/lib/python3.13/site-packages/pyproject_api/_backend.py True flit_core.buildapi
py312: install_package> python -I -m pip install --force-reinstall --no-deps /Users/carl/work/apio/.tox/.tmp/package/8/apio-0.9.6.tar.gz
py312: commands[0]> python -m pytest --cov --cov-report=html apio test --offline
================================================================================================================ test session starts ================================================================================================================
platform darwin -- Python 3.12.8, pytest-8.3.3, pluggy-1.5.0
cachedir: .tox/py312/.pytest_cache
rootdir: /Users/carl/work/apio
configfile: pyproject.toml
plugins: cov-5.0.0
collected 60 items

test/commands/test_boards.py .. [ 3%]
test/commands/test_build.py ... [ 8%]
test/commands/test_clean.py ... [ 13%]
test/commands/test_create.py . [ 15%]
test/commands/test_drivers.py . [ 16%]
test/commands/test_examples.py . [ 18%]
test/commands/test_fpgas.py .. [ 21%]
test/commands/test_graph.py .. [ 25%]
test/commands/test_install.py . [ 26%]
test/commands/test_lint.py . [ 28%]
test/commands/test_modify.py . [ 30%]
test/commands/test_packages.py . [ 31%]
test/commands/test_report.py .. [ 35%]
test/commands/test_sim.py . [ 36%]
test/commands/test_system.py . [ 38%]
test/commands/test_test.py . [ 40%]
test/commands/test_time.py .. [ 43%]
test/commands/test_uninstall.py . [ 45%]
test/commands/test_upgrade.py s [ 46%]
test/commands/test_upload.py ... [ 51%]
test/commands/test_verify.py . [ 53%]
test/integration/test_examples.py s [ 55%]
test/integration/test_packages.py s [ 56%]
test/integration/test_projects.py ss [ 60%]
test/integration/test_utilities.py s [ 61%]
test/managers/test_scons_args.py . [ 63%]
test/managers/test_scons_filters.py . [ 65%]
test/scons/test_scons_util.py ............... [ 90%]
test/test_apio.py .. [ 93%]
test/test_apio_context.py . [ 95%]
test/test_cmd_util.py . [ 96%]
test/test_util.py .. [100%]

---------- coverage: platform darwin, python 3.12.8-final-0 ----------
Coverage HTML written to dir htmlcov

=========================================================================================================== 54 passed, 6 skipped in 2.03s ===========================================================================================================
.pkg: _exit> python /Users/carl/opt/miniconda3/envs/icestudio/lib/python3.13/site-packages/pyproject_api/_backend.py True flit_core.buildapi
py312: OK (6.01=setup[3.76]+cmd[2.24] seconds)
congratulations :) (6.04 seconds)

@cavearr
Copy link
Member

cavearr commented Dec 16, 2024

@zapta i'm testing apio from console outside icestudio and all works well with white spaces in paths, in fact i need remove the check of spaces in the apio path because this is important to support , there are a lot of examples and people use directorios with spaces, i prefer that you remove this check from apio.

The think that fails is when apio dir is inside a path with spaces, this only affect for development or for users with usernames with spaces, i think this check could be fine (check if apio_home has spaces, but not the project dir that works well). We could maitain open the issue for APIO_HOME with spaces while verilator support it, but this is not important if we advice to the user.

What do you think? could do you do it?

@zapta
Copy link
Collaborator Author

zapta commented Dec 16, 2024

@cavearr, the --offline causes the real tests to be skipped (notice the 's' marks in the integration tests). Try to run make check which is the standard way we check before submitting.

I can remove the two checks and leave only for the apio home path. Will also modify the test such that those two paths have contains white spaces.

@cavearr
Copy link
Member

cavearr commented Dec 16, 2024

Thanks @zapta, running "make check" and review errors!

Testing boards with apio commands all works well with spaces in the path (with apio installed in APIO_HOME without spaces) as i said before, is very important that this could work as now, and appears works well.

@cavearr
Copy link
Member

cavearr commented Dec 17, 2024

@zapta i need unblock the constraint of apio when detect spaces in paths, if you couldn't today i could do it without problems, only tell me ok?

@zapta
Copy link
Collaborator Author

zapta commented Dec 17, 2024

Hi @cavearr, please do.

@cavearr
Copy link
Member

cavearr commented Dec 17, 2024

Thanks, this is in my side!

@zapta
Copy link
Collaborator Author

zapta commented Dec 17, 2024 via email

@cavearr
Copy link
Member

cavearr commented Dec 17, 2024

Hi @zapta i have fix the problem with spaces in path (for tests).

Check test_examples.py file, at Apio we should adapt the path management like the example i fix it. The only way in that all paths works is with pathlib library, please check the file to view it.

In other way i think we need to change the path creation from something like:

base_path / "examples" / "Alhambra-II" / "ledon" / "ledon.v"

to solution with os.path.join or similar because we need to do multi platform paths (Windows).

@zapta
Copy link
Collaborator Author

zapta commented Dec 18, 2024

Hi @cavearr, I believe that that the / is a redefinition of the div operator for the Path class, that accepts a string as an argument and return a new joined Path. That is, it's not a simple concatenation using the "/" string, but an abstracted path joining using an operator that was designed for it.

https://stackoverflow.com/questions/53083963/python-pathlib-operator-how-does-it-do-it

If you have an example that doesn't behave well please let me know.

@zapta
Copy link
Collaborator Author

zapta commented Dec 22, 2024

All items got unblocked. Thanks.

Closing.

@zapta zapta closed this as completed Dec 22, 2024
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

No branches or pull requests

3 participants