-
Notifications
You must be signed in to change notification settings - Fork 1
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
.github/workflows/tests: Enable ECL tests. #37
base: master
Are you sure you want to change the base?
Conversation
I am still not convinced about the benefits for the reasons below. Nonetheless, I don't think this is very important so I'm leaving my thoughts as a note for future reference and reflections. I think we need to distinguish two sets of goals: those of Nyxt and those of our libraries for their own sake. It should be noted that the success of the former is what makes the latter possible, as @jmercouris will certainly agree. In light of the above and, regarding the CL implementation/OS CI matrix, I believe that GNU/Linux+SBCL would suffice for Nyxt's goals. If we think about the library alone, I see value when testing on multiple CL implementations (SBCL+CCL to be specific). Regarding the argument that the CI useful for day-to-day work (such as #36), I'm afraid I still don't quite see it. The CI is triggered on push, so the experience differs from running a CL implementation locally - incremental development or the ability to inspect and debug test suite runs, etc. This is general observation on CI. The following argument is subtle but important (while not completely related). Notice that the testing CI environment relies on the libraries' submodules. To ensure that it is faithful to Nyxt's testing environment, these need to be kept in sync (in turn, Nyxt's git submodules should be kept in sync with Guix). How do I, as a Nyxt reviewer, test changes made to a library? I run the library test suite within Nyxt's dev environment ( I also have doubts regarding the no maintenance argument (beyond the point above). The CI relies on Roswell (which, from my personal experience, is a can of worms) and Github Actions APIs. That does require some work at times. Finally, I'd like to highlight some "soft" arguments. I believe we should be very good at doing one single thing alone before attempting to do many. Adding each source of complexity one step at a time seems like a reasonable strategy. Also, experimentation is key to reach success. If this change is indeed something that we are lacking, could we not wait until it becomes evident that we need to bring it back? It may be debatable but, in this concrete case and to my mind, I don't think we have reached that point to re-evaluate the decision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do agree on going forward with this PR, I've left some minor suggestions. Thanks!
# Use ccl-bin/1.12.1 instead of 'ccl' because of | ||
# https://github.com/roswell/roswell/issues/534. | ||
# TODO: Revert when Roswell is functional again. | ||
lisp: [sbcl-bin, ccl-bin/1.12.1, ecl/21.2.1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest giving a try with ccl-bin
alone, as I think it will work now (I did try it in the past if I'm not mistaken). If it does work, the comments are no longer needed.
I'd suggest not testing on ecl since Roswell doesn't provide an ecl-bin
(which would be installed faster). Also, as you can see, macOS+ECL takes 16 minutes to pass (which I think it's too much).
# https://github.com/roswell/roswell/issues/534. | ||
# TODO: Revert when Roswell is functional again. | ||
lisp: [sbcl-bin, ccl-bin/1.12.1, ecl/21.2.1] | ||
rosargs: [dynamic-space-size=3072] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flag isn't needed in the context of this library. We use it in Nyxt only.
# TODO: Revert when Roswell is functional again. | ||
lisp: [sbcl-bin, ccl-bin/1.12.1, ecl/21.2.1] | ||
rosargs: [dynamic-space-size=3072] | ||
os: [ubuntu-latest, macos-latest] # try windows-latest when we understand commands to install Roswell on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmercouris has suggested that we should refrain from these kind of comments for future work, which I tend to agree.
rosargs: [dynamic-space-size=3072] | ||
os: [ubuntu-latest, macos-latest] # try windows-latest when we understand commands to install Roswell on it | ||
|
||
# run the job on every combination of "lisp" and "os" above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we don't need this comment.
|
Needed for #36.
Rationale: