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

Upgrade React Router to V6 #1054

Closed
wants to merge 49 commits into from
Closed

Conversation

EshaanAgg
Copy link
Contributor

@EshaanAgg EshaanAgg commented Jun 12, 2024

Upgrades from React Router V5 to React Router V6.

The primary changes included in this PR:

  • Converted the App component from a class component to a functional component:
    I did it while experimenting with different ways to use hooks like useNavigate and useLocation in the Overview component. In the end, I did not need it, but it is a good change to keep in my opinion.
  • Changed the Switch components to Route components
  • Changed all the existing instances of using the history object to other means

Initially, I tried converting the Overview component to a functional one. Still, the same resulted in many breaking changes as I am unfamiliar with what and how each function works. I think so I have found a way that allows us to continue using class components, which integrating it with hooks, that doesn't involve too much of a rewrite and refactoring.

Fixes #805.

@EshaanAgg
Copy link
Contributor Author

The immediate issue I am facing is that the tests for QuantilePlot seem to fail now and I have no clue what is going wrong with it. Would really appreciate any help with the same.

@PhilippWendler PhilippWendler marked this pull request as draft June 13, 2024 13:41
@PhilippWendler PhilippWendler changed the title wip: Upgrade React Router to V6 Upgrade React Router to V6 Jun 13, 2024
@PhilippWendler
Copy link
Member

Seems to work seamlessly in my first tests. Very nice! And a much smaller PR than I had feared. :-)

The immediate issue I am facing is that the tests for QuantilePlot seem to fail now and I have no clue what is going wrong with it. Would really appreciate any help with the same.

There are several errors with the message "Error: Not implemented: navigation (except hash changes)" shown in the log. My guess would be that something attempts to change the URL. There are also stacktraces available for these errors that should help.

@EshaanAgg
Copy link
Contributor Author

Yes sir. I have been trying to figure out this error and tried again all the solutions that I had researched when I faced this problem earlier while working on different PR, but to no avail. I have some other ideas and hope to complete my experimentation with them by this weekend. I will keep adding updates on my progress here.

@EshaanAgg
Copy link
Contributor Author

Update: I got the URL navigation working! We are on a particular version of Jest, which doesn't allow overwriting the window.location object. Still, we can use the window.history.pushState function to set the value of the window.location.href as needed. All the tests now run successfully :)

There is another minor hiccup, though. Some snapshots for the Quantile.js component seem to have changed, but I have not changed the component's markup. I tried manually using the various interactions mocked in the test in the browser, and they seem correct. I have the following questions now:

  • Is the change in markup possible because we introduced the NavSync component in the Overview component?
  • Is there a way to visually check the Quantile snapshots and the current renders so we can see the difference between them and compare them?
  • Can anybody else set up this branch locally, test out the Quantile tab, and let me know if everything behaves as expected? If it does, then I feel we can update the snapshots with the current render.

@PhilippWendler
Copy link
Member

Update: I got the URL navigation working! We are on a particular version of Jest, which doesn't allow overwriting the window.location object. Still, we can use the window.history.pushState function to set the value of the window.location.href as needed. All the tests now run successfully :)

🎉

There is another minor hiccup, though. Some snapshots for the Quantile.js component seem to have changed, but I have not changed the component's markup. I tried manually using the various interactions mocked in the test in the browser, and they seem correct. I have the following questions now:

  • Is the change in markup possible because we introduced the NavSync component in the Overview component?

Hm. Could be, but if I look at the start of the changes I see that the test now renders a different column in the plot. This might look more like if some state change is not picked up? Note that the test is named "Quantile Plot should match HTML snapshot with selection of the type status" and we previously had the status column, but now there is a different column.

  • Is there a way to visually check the Quantile snapshots and the current renders so we can see the difference between them and compare them?

Not that I am aware of. Maybe google for this?

  • Can anybody else set up this branch locally, test out the Quantile tab, and let me know if everything behaves as expected? If it does, then I feel we can update the snapshots with the current render.

I think it is more likely that the test is not behaving correctly right now due to the mismatch between test name and test result.

@EshaanAgg
Copy link
Contributor Author

Got it sir. Will try to fix this as soon as possible.

@EshaanAgg EshaanAgg marked this pull request as ready for review June 18, 2024 12:13
@EshaanAgg
Copy link
Contributor Author

I have fixed the errors in the test and the application. This PR is now ready for review!

@PhilippWendler
Copy link
Member

Thanks a lot!

I found two related problems with state updates. If I configure a filter in the filter sidebar and keep the sidebar open while changing tabs, the filter is lost. Similarly, if I change a filter via the drop down and then change tabs, the filter is lost. Here is a video of the former case:
Bildschirmaufzeichnung vom 18.06.2024, 16:30:33.webm

Could you please have a look?

Aside from this, everything seems to work fine. 👍

@EshaanAgg
Copy link
Contributor Author

EshaanAgg commented Jun 18, 2024

Okay, sir! I can confirm that these bugs are replicable on my end as well. Will try to patch the same and will update here.

@EshaanAgg EshaanAgg requested a review from PhilippWendler June 19, 2024 09:10
@EshaanAgg
Copy link
Contributor Author

@PhilippWendler This is ready for review.

@PhilippWendler
Copy link
Member

I still get some weird behavior when clicking around with filters. For example, the drop down filter first doesn't update and then the UI seems to lag one filter behind what is actually being applied as a filter. Similar with the sidebar. Can you reproduce or should I make a video?

PhilippWendler and others added 27 commits June 28, 2024 12:18
On Ubuntu since 24.04, user namespaces are forbidden for regular users
(cf. sosy-lab#1041 and sosy-lab#1042).
There is a global sysctl switch to enable them again,
but applications whose AppArmor profile allows this can also use it.
(Typically, AppArmor only restricts application,
but in this case an AppArmor profile can actually provide a privilege
than an unconfined application does not have.)
More explanations are at
https://ubuntu.com/blog/ubuntu-23-10-restricted-unprivileged-user-namespaces

In order to make BenchExec usable out-of-the-box after installing
the .deb package we want to ship such an AppArmor profile.
This is made complicated by the fact that the AppArmor profile
that is necessary on Ubuntu 24.04+
breaks AppArmor on previous Ubuntu versions.
So we have to install this profile conditionally.
I found a way to do so using ucf (a tool for handling config files)
and this seems to work in my tests on Ubuntu 22.04 (old AppArmor),
Ubuntu 24.04 (new AppArmor), and Debian 12 (old AppArmor),
as well as installation without AppArmor present.

There are two known remaining problems:
- If one upgrades from Ubuntu 22.04 to Ubuntu 24.04 while having
  BenchExec installed, the AppArmor profile will not be installed,
  so BenchExec will not work.
  Upgrading or reinstalling the BenchExec package makes it work.
- The command "python3 -m benchexec.test_tool_info" will not work,
  because the AppArmor profile won't match it.
  One has to either disable container mode or temporarily allow
  the use of user namespaces for the whole system.
  If we implement sosy-lab#1053 this would just work.

Part of sosy-lab#1041.
This was a race condition if a process in the same cgroup as BenchExec
stopped while we check its status.
…default and SV-COMP wrapper

In `executable()`, we look for `bin/sea_svcomp` (the SV-COMP wrapper) first.
If it does not exist, then we look for `bin/sea` (the default wrapper).
CPAchecker will use bin/cpachecker instead of scripts/cpa.sh soon
(cf. https://gitlab.com/sosy-lab/software/cpachecker/-/issues/632).
We will support both in the tool-info module.

Same change is also done for CPA-Witness2Test,
which also has its executable renamed.
CPAchecker will soon use command-line arguments with "--" as prefix.
The old arguments will continue to be supported,
but we want to use the new ones in the tool-info module when possible
in order to not confuse users.
So let's let the tool-info module adapt to the options
that the user specified in the benchmark definition.

Cf. https://gitlab.com/sosy-lab/software/cpachecker/-/issues/1218
Previously, the tool-info module of CPAchecker ensured
that the executable of CPA-witness2test is covered by program_files(),
because the scripts/ directory is part of the program files.
But for new versions of CPAchecker we do not add all of bin/
to the program files, so we need to explicitly add cpa-witness2test.
Since version 69.3.0 the names of the produced build artifacts
changes, but we prefer to keep the old names for compatibility.
SMT-COMP started to use BenchExec this year,
and for Termination Competition is an important part of their
measurement setup:
https://lists.rwth-aachen.de/hyperkitty/list/[email protected]/thread/6RTY4BDARIS74HSSUDJQYYQNURHIZ7IR/
As announced in the changelog of BenchExec 3.22 and on the issue tracker.

Part of sosy-lab#986.
According to the man page debhelper-compat-upgrade-checklist
level 11 is deprecated and not recommended.
A test build on level 12 produces an identical .deb package (same hash),
so it seems we can safely upgrade.

Also switch how we specify the compatibility level:
In debian/control instead of debian/compat,
because right now we need to specify it in two places.
This makes it work without requiring sudo.
We have a heuristic that checks if CPAchecker is used from source
but misses a rebuild. This would crash if broken symlinks or other
file-access problems occur.
As it is just an optional feature, we can ignore such problems
and print a warning.

An example where this happened was for example
system-wide installation of CPAchecker
with the executable being /usr/bin/cpachecker
and /usr/src existing.
We do not need to scan through the src directory of CPAchecker
if we are not going to use the information anyway.
So check this first.
@EshaanAgg
Copy link
Contributor Author

I seem to have messed up the git history of this branch (I rebased main into it instead of merging it, and thus, now all previous commits show me as an author as well). Will close this PR, and open a new one so that the history is maintained. Apologies for the same!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Upgrade to react-router 6
4 participants