Conversation
|
I have no idea how this could possibly be passing, given Ubuntu 24.04 has the version of pip that gives an error if you run it as root and try to install packages system-wide. Unless something in the install scripts is setting the special environment variable that masks that error? |
p4studio installs python packages locally in the |
fruffy
force-pushed
the
fruffy/ubuntu24_04
branch
from
567017a to
d54dc34
Compare
fruffy
force-pushed
the
fruffy/ubuntu24_04
branch
from
d54dc34 to
0e01e27
Compare
fruffy
force-pushed
the
fruffy/ubuntu24_04
branch
from
20287ab to
408a9ee
Compare
|
@jafingerhut This finally allows us to build Ubuntu 24.04, GPT 5.3 was a great help. |
|
If @grg or @ChrisDodd are interested in giving this a look, that would be great. Understood if they would prefer not to, also. Maybe give one of them until Feb 25 or so to take a look and comment, and I'll approve then. I guess most of the C++ code changes are due to needing to support a newer version of GCC? |
Yes, also rapidjson and fmt are quite out of date. The logging fixes are mostly guesses. |
|
I'll try to take a look in the next few days. Sorry for missing this earlier. |
fruffy
force-pushed
the
fruffy/ubuntu24_04
branch
from
7223262 to
0bf8121
Compare
ChrisDodd
left a comment
ChrisDodd
left a comment
There was a problem hiding this comment.
I don't see anything that looks wrong, but I'm not very knowledgeable about CMAKE or the details of how to make subtle things work. The C++ changes are all fine, though.
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
fruffy
force-pushed
the
fruffy/ubuntu24_04
branch
from
0bf8121 to
3155668
Compare
grg
left a comment
grg
left a comment
There was a problem hiding this comment.
Looks okay. My comments are all questions -- only one of which needs confirmation before merging.
| -DSRC_BUILD=${CMAKE_CURRENT_BINARY_DIR}/bf_kdrv.ko | ||
| -DSRC_SOURCE=${CMAKE_CURRENT_SOURCE_DIR}/bf_kdrv.ko | ||
| -DDST=${DRIVER_FILE} |
There was a problem hiding this comment.
SRC_BUILD and DST both end up pointing to the same file. Is this intentional? (I believe it is, but just want to confirm. The copy script will skip the copy if it falls back to using SRC_BUILD since the ONLY_IF_DIFFERENT flag is passed to the file(COPY_FILE ...) command.)
There was a problem hiding this comment.
Yes, initially I tried to have a separate build, but ended up running into a bunch of issues. Ultimately, I ended up handing this to GPT 5.3... This build works for me and handles both Ubuntu versions for the time being.
| set(KBUILD_CMD $(MAKE) -C ${KERNELDIR} M=${CMAKE_CURRENT_SOURCE_DIR} MO=${CMAKE_CURRENT_BINARY_DIR} modules) | ||
| set(KBUILD_CLEAN $(MAKE) -C ${KERNELDIR} M=${CMAKE_CURRENT_SOURCE_DIR} MO=${CMAKE_CURRENT_BINARY_DIR} clean) |
There was a problem hiding this comment.
Question (no change desired in this PR): do we have any expectation that this would work if someone uses ninja as their generator instead of make? (i.e., cmake -GNinja ...)
There was a problem hiding this comment.
As far as I know, no. I have only seen make used with these modules
| } | ||
|
|
||
| template <typename T> | ||
| std::string stream_to_string(const T &value) { |
There was a problem hiding this comment.
Why did this function (and all the calls to it) need to be added? Were there some implicit casts that were failing?
There was a problem hiding this comment.
The fmt version used is quite old, it seems to have problems with newer versions of gcc for some of these constructs that use .str(). I have not had the time to dig deeper why. Updating the vendored fmt might also solve the issue.
jafingerhut
left a comment
jafingerhut
left a comment
There was a problem hiding this comment.
Approving to unblock merging, based on Glen's review.
Signed-off-by: fruffy <fruffy@nyu.edu> Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
4 participants
No description provided.