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

Remove the check for sequence number from P2TargetUtils.checkProfile #1541

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Dec 31, 2024

Currently it is assumed that a profile is up-to-date if the sequence number does not change, but this has some risk to produce strange errors if for some reason there is no number or it is not changed. And even if the number changed, we still perform a check for actual changes.

Because of the constant problems in inconsistent behavior this now completely removes the check for the sequence number from the profile.

Currently it is assumed that a profile is up-to-date if the sequence
number does not change, but this has some risk to produce strange errors
if for some reason there is no number or it is not changed. And even if
the number changed, we still perform a check for actual changes.

Because of the constant problems in inconsistent behavior this now
completely removes the check for the sequence number from the profile.
Copy link

Test Results

   285 files  ±0     285 suites  ±0   52m 12s ⏱️ -57s
 3 586 tests ±0   3 510 ✅ ±0   76 💤 ±0  0 ❌ ±0 
10 950 runs  ±0  10 719 ✅ ±0  231 💤 ±0  0 ❌ ±0 

Results for commit cd9649d. ± Comparison against base commit 6e21922.

@HannesWell
Copy link
Member

In general this looks good and I appreciate the simplification.

But I wonder why this succeeds while #1540 fails a test. If possible I think it would better to make #1540 succeed as a whole.

@laeubi
Copy link
Contributor Author

laeubi commented Dec 31, 2024

As both of @HannesWell and @merks expressed consensus here already that it is the right direction:

and no test are broken I will merge this now so we can see if anything goes wrong with this smaller change.

@laeubi laeubi merged commit 91cdb59 into eclipse-pde:master Dec 31, 2024
18 checks passed
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

Successfully merging this pull request may close these issues.

2 participants