M-Labs/nix-servo
4
1
Fork 2
Code Issues 1 Pull Requests 1 Packages Projects Releases Wiki Activity

Add fast-servo linien gateware #37

Merged
sb10q merged 4 commits from fsagbuya/nix-servo:gateware into master 2024-03-01 16:39:57 +08:00
Conversation 16 Commits 4 Files Changed 19 +11 -2
1 changed files with 11 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ad725ae41b - Show all commits

13
flake.nix
View File

@ -202,11 +202,20 @@
fast-servo/linien-fast-servo-server.patch
];
fsagbuya marked this conversation as resolved Outdated
sb10q commented 2024-02-28 13:11:47 +08:00
Outdated
Review
Copy Link

To which patch does this commit refer to?

To which patch does this commit refer to?
sb10q commented 2024-02-28 13:12:13 +08:00
Outdated
Review
Copy Link

How does it relate to the provenance information in the patches?

How does it relate to the provenance information in the patches?
fsagbuya commented 2024-02-28 13:15:15 +08:00
Outdated
Review
Copy Link

Will clean this up and update to the match the provenance info from patches.

Will clean this up and update to the match the provenance info from patches.
fsagbuya commented 2024-02-28 13:19:18 +08:00
Outdated
Review
Copy Link

Removed the comment instead, since it will be described from the provenance info.

Removed the comment instead, since it will be described from the provenance info.
nativeBuildInputs = [
(pkgs.python3.withPackages(ps: [ migen misoc ps.linien-common ]))
(pkgs.python3.withPackages(ps: [
migen misoc
(ps.linien-common.overrideAttrs(oa: {
fsagbuya marked this conversation as resolved Outdated
sb10q commented 2024-02-26 16:33:26 +08:00
Outdated
Review
Copy Link

What needs that?

What needs that?
fsagbuya commented 2024-02-26 16:40:35 +08:00
Outdated
Review
Copy Link

It is a fix to prevent this error:

PermissionError: [Errno 13] Permission denied: '/homeless-shelter'
It is a fix to prevent this error: ``` PermissionError: [Errno 13] Permission denied: '/homeless-shelter' ```
sb10q commented 2024-02-26 16:41:29 +08:00
Outdated
Review
Copy Link

I've been using Nix for 6 years and I had already guessed that much.

I've been using Nix for 6 years and I had already guessed that much.
# config.py tries to access $HOME, but we do not need it for building gateware
postPatch = ''
fsagbuya marked this conversation as resolved Outdated
sb10q commented 2024-02-29 16:15:00 +08:00
Outdated
Review
Copy Link

You have not answered the question about which part of the build system needs $HOME.

You have not answered the question about which part of the build system needs $HOME.
fsagbuya commented 2024-02-29 17:49:29 +08:00
Outdated
Review
Copy Link

According to the trace log:

FileNotFoundError: [Errno 2] No such file or directory: '/homeless-shelter/.local/share/linien'

It is found on this part of the build system:
https://github.com/elhep/linien/blob/fast_servo_merging/linien-common/linien_common/config.py#L27

According to the trace log: ``` FileNotFoundError: [Errno 2] No such file or directory: '/homeless-shelter/.local/share/linien' ``` It is found on this part of the build system: https://github.com/elhep/linien/blob/fast_servo_merging/linien-common/linien_common/config.py#L27
sb10q commented 2024-02-29 18:22:46 +08:00
Outdated
Review
Copy Link

Do we actually need this stuff to build the gateware or can config.py be replaced with an empty/dummy file?

Do we actually need this stuff to build the gateware or can config.py be replaced with an empty/dummy file?
fsagbuya commented 2024-02-29 18:39:28 +08:00
Outdated
Review
Copy Link

I think it can be replaced with an empty/dummy file, will test and have it added to the patch.

I think it can be replaced with an empty/dummy file, will test and have it added to the patch.
fsagbuya commented 2024-03-01 11:49:31 +08:00
Outdated
Review
Copy Link

Update: removed the linien-common dependency (which imports config.py) from the gateware build

Update: removed the `linien-common` dependency (which imports `config.py`) from the gateware build
sb10q commented 2024-03-01 12:05:28 +08:00
Outdated
Review
Copy Link

What exactly did you change?

What exactly did you change?
fsagbuya commented 2024-03-01 12:24:40 +08:00
Outdated
Review
Copy Link

Added a new commit for the changes. Converted the variables to its actual value in linien_common and config.py, so that we can omit the dependency for this use case.

Added a new commit for the changes. Converted the variables to its actual value in `linien_common` and `config.py`, so that we can omit the dependency for this use case.
sb10q commented 2024-03-01 13:37:35 +08:00
Outdated
Review
Copy Link

That's much worse than even the one line of code that sets $HOME. I had suggested to edit config.py (and not other files), what is wrong with this approach?

That's much worse than even the one line of code that sets $HOME. I had suggested to edit config.py (and not other files), what is wrong with this approach?
fsagbuya commented 2024-03-01 14:46:55 +08:00
Outdated
Review
Copy Link

The config.py file that requires modification is part of the linien-common dependency:

nativeBuildInputs = [
          (pkgs.python3.withPackages(ps: [ migen misoc ps.linien-common ]))

To implement the necessary changes, we need to apply an override/overlay for the linien-common package in the upstream. Would this approach be acceptable?

The config.py file that requires modification is part of the linien-common dependency: ``` nativeBuildInputs = [ (pkgs.python3.withPackages(ps: [ migen misoc ps.linien-common ])) ``` To implement the necessary changes, we need to apply an override/overlay for the linien-common package in the upstream. Would this approach be acceptable?
sb10q commented 2024-03-01 14:49:38 +08:00
Outdated
Review
Copy Link

You can submit a clean patch upstream so building gateware doesn't need config.py/$HOME. And in the meantime something like:

(pkgs.python3.withPackages(ps: [ migen misoc (ps.linien-common.overrideAttrs(oa: {
   # config.py tries to access $HOME, but we do not need it for building gateware
   postPatch = "echo > config.py";  
   doCheck = false;
}))
]))
You can submit a clean patch upstream so building gateware doesn't need config.py/$HOME. And in the meantime something like: ``` (pkgs.python3.withPackages(ps: [ migen misoc (ps.linien-common.overrideAttrs(oa: { # config.py tries to access $HOME, but we do not need it for building gateware postPatch = "echo > config.py"; doCheck = false; })) ])) ```
fsagbuya commented 2024-03-01 16:12:00 +08:00
Outdated
Review
Copy Link

Updated: Also need to modify the file __init__.py :

    >   File "/nix/store/04nl55kmfkaacbv7vdm18gzhyl586r4r-python3.11-linien-common-1.0.1/lib/python3.11/site-packages/linien_common/__init__.py", line 6, in <module>
       >     from .config import LOG_FILE_PATH
       > ImportError: cannot import name 'LOG_FILE_PATH' from 'linien_common.config'

For this temporary fix, it is also set to an empty file. Other approach is to create a patch that removes related lines to config.py import.

Updated: Also need to modify the [file](https://github.com/linien-org/linien/blob/master/linien-common/linien_common/__init__.py#L6) `__init__.py `: ``` > File "/nix/store/04nl55kmfkaacbv7vdm18gzhyl586r4r-python3.11-linien-common-1.0.1/lib/python3.11/site-packages/linien_common/__init__.py", line 6, in <module> > from .config import LOG_FILE_PATH > ImportError: cannot import name 'LOG_FILE_PATH' from 'linien_common.config' ``` For this temporary fix, it is also set to an empty file. Other approach is to create a patch that removes related lines to config.py import.
sb10q commented 2024-03-01 16:16:36 +08:00
Outdated
Review
Copy Link

Aaah, the joys of physicist code. Removing the contents of that poorly designed __init__.py sounds fine to me as well.

Aaah, the joys of physicist code. Removing the contents of that poorly designed ``__init__.py`` sounds fine to me as well.
🚀 1
echo > linien_common/config.py
echo > linien_common/__init__.py
'';
doCheck = false;
fsagbuya marked this conversation as resolved Outdated
sb10q commented 2024-03-01 16:15:25 +08:00
Outdated
Review
Copy Link

Indentation/style. My comment was just a hint, not a textual example.

Indentation/style. My comment was just a hint, not a textual example.
👍 1
}))
]))
fsagbuya marked this conversation as resolved Outdated
sb10q commented 2024-02-28 18:11:37 +08:00
Outdated
Review
Copy Link

The directory name inconsistency in these two lines is very suspicious.

The directory name inconsistency in these two lines is very suspicious.
fsagbuya commented 2024-02-29 10:25:49 +08:00
Outdated
Review
Copy Link

This is linien's default output directory for the bin file. I agree this is suspicious. Will add a patch to use the same directory for the top.bit.

This is [linien's default output directory](https://github.com/elhep/linien/blob/fast_servo_merging/gateware/fpga_image_helper.py#L101-L103) for the bin file. I agree this is suspicious. Will add a patch to use the same directory for the top.bit.
sb10q commented 2024-02-29 10:41:20 +08:00
Outdated
Review
Copy Link

If this is an upstream issue then fine. Just want to make sure it's understood and not introduced here.

If this is an upstream issue then fine. Just want to make sure it's understood and not introduced here.
👍 1
vivado
];
fsagbuya marked this conversation as resolved Outdated
sb10q commented 2024-02-28 18:10:23 +08:00
Outdated
Review
Copy Link

Why is it top.bit and gateware.bin? Why not top.bin?

Why is it top.bit and gateware.bin? Why not top.bin?
sb10q commented 2024-02-28 18:10:37 +08:00
Outdated
Review
Copy Link

(Assuming we need .bin at all)

(Assuming we need .bin at all)
fsagbuya commented 2024-02-29 10:26:34 +08:00
Outdated
Review
Copy Link

Will rename it as top.bin.

Will rename it as top.bin.
sb10q commented 2024-02-29 10:43:49 +08:00
Outdated
Review
Copy Link

#37 (comment)

https://git.m-labs.hk/M-Labs/nix-servo/pulls/37#issuecomment-9150
fsagbuya commented 2024-02-29 10:45:56 +08:00
Outdated
Review
Copy Link

Okay. Will retain it as is.

Okay. Will retain it as is.
buildPhase = ''
export HOME=$(mktemp -d)
python -m gateware.fpga_image_helper -p fastservo
'';
installPhase = ''