Skip to content

Fix: circleci duplicate key path #115

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

Merged
merged 4 commits into from
Jul 27, 2021

Conversation

ParthPratim
Copy link
Contributor

@ParthPratim ParthPratim commented Jun 1, 2021

Signed-off-by: Parth Pratim Chatterjee [email protected]

Attempts to fix circleci build error with duplicate path

Reference : https://circleci.com/docs/2.0/artifacts/

This PR was made as a part of Google Summer of Code 2021 for the project Direct Code Execution Modernization

@ParthPratim
Copy link
Contributor Author

ParthPratim commented Jun 1, 2021

I think with this fix most of the problems would be resolved with @tomhenderson Sir's PR #114 as now circleci fails only because of some wrong function definitions in ipv6-linux.cc/h. (Please check the ci details below)

ubuntu 14.04 and 17.04 fail in apt-get update as they are no longer supported.
We could remove these docker images from the build as a solution to it.

Also, should I add support for ubuntu 20.04 in our circleci and make a docker and put a PR here https://github.com/direct-code-execution/dce-dockerfiles ?

Please let me know if there is something else to be done.

@teto
Copy link
Member

teto commented Jun 1, 2021

Let's drop 14.04/17.04. It would be nice to have 20.04 but let's put it in a separate PR. Also some of the CI failures mention it can't find include header . It would be nice to have it green when merging, even if it means another PR cherry-picks this branch.

@ParthPratim
Copy link
Contributor Author

ParthPratim commented Jun 2, 2021

The missing header files is specifically a problem with the fedora environment. I'll try to take a look into it by manually installing the docker and setting up the dce environment.

Also, fedora 26/27 have both reached end of life long back, so maybe the toolchain versions might be causing the problem, but I'm not sure about it. I'll take a look at it.

@tomhenderson
Copy link
Collaborator

The missing header files is specifically a problem with the fedora environment. I'll try to take a look into it by manually installing the docker and setting up the dce environment.

Also, fedora 26/27 have both reached end of life long back, so maybe the toolchain versions might be causing the problem, but I'm not sure about it. I'll take a look at it.

Is there any reason not to drop fedora26, 27, Ubuntu 14.04, and Ubuntu 17.04, and just keep Ubuntu 16.04 (for the moment)? All of these (including Ubuntu 16.04) are end-of-life although Ubuntu 16.04 can be kept going for now with Canonical ESM support. Then, once Ubuntu 16.04 tests are clean and Ubuntu 20.04 is ready, we add Ubuntu 20.04 and drop 16.04? Note that Ubuntu 16.04's toolchain is already starting to be a hassle for ns-3 to support.

@teto
Copy link
Member

teto commented Jun 2, 2021

No problem for me. I just would prefer to actually have a green CI when merging, even if it means this is cherry-picked into another branch,

@ParthPratim
Copy link
Contributor Author

Even after I removed the jobs from the config.yaml file, they still seem to be looked for, to build on circleci instance, and they fail as it cannot find a job definition in the config file.

Probably, specific jobs should be disabled through the circleci admin interface or the github webhooks page.

@teto
Copy link
Member

teto commented Jun 3, 2021

try with

diff --git a/.circleci/config.yml b/.circleci/config.yml
index 4ecc6e3..eb8de1a 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -104,9 +104,7 @@ workflows:
   version: 2
   build:
     jobs:
-     - ubuntu14.04
      - ubuntu16.04
-     - ubuntu17.04
      - fedora26
      - fedora27
   nightly_workflow:

@teto
Copy link
Member

teto commented Jun 3, 2021

can you squash the commits please ? I think the most interesting would be for @tomhenderson to cherry-pick the squashed commit into #114, hopefully we can have a green CI then.

@ParthPratim ParthPratim force-pushed the circleci-fix branch 2 times, most recently from dbb6b1b to 747c3b5 Compare June 5, 2021 17:41
jobs:
- ubuntu14.04_valgrind
- ubuntu14.04_valgrind
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it strange to keep that job ifor 14.04 ?

Signed-off-by: Parth Pratim Chatterjee <[email protected]>
@teto
Copy link
Member

teto commented Jul 26, 2021

what's the status of this ?

[121/390] Compiling myscripts/ns-3-dce-umip/test/dce-umip-test.cc
../myscripts/ns-3-dce-umip/test/dce-umip-test.cc: In member function 'virtual void ns3::DceUmipTestCase::DoRun()':
../myscripts/ns-3-dce-umip/test/dce-umip-test.cc:234:27: error: 'Default' is not a member of 'ns3::YansWifiPhyHelper'
   YansWifiPhyHelper phy = YansWifiPhyHelper::Default ();
                           ^
../myscripts/ns-3-dce-umip/test/dce-umip-test.cc:240:45: warning: 'virtual void ns3::WifiHelper::SetStandard(ns3::WifiPhyStandard)' is deprecated [-Wdeprecated-declarations]
   wifi.SetStandard (WIFI_PHY_STANDARD_80211a);
                                             ^
In file included from ../myscripts/ns-3-dce-umip/test/dce-umip-test.cc:50:0:
/home/ns3dce/build/include/ns3-dev/ns3/wifi-helper.h:502:16: note: declared here
   virtual void SetStandard (WifiPhyStandard standard);
                ^

@tomhenderson
Copy link
Collaborator

This has been fixed and pushed. I am only experiencing one lingering issue with umip:
direct-code-execution/ns-3-dce-umip#2

It might possibly help for Parth to rebase this PR to trigger another attempt with CircleCi?

Signed-off-by: Parth Pratim Chatterjee <[email protected]>
@ParthPratim
Copy link
Contributor Author

@tomhenderson Sir, I tried to push a commit to trigger a build, but it still seems to fail at the same point where Matt Sir mentioned.
Is it because I'm using our gitlab bake repository as compared to out github repository, because other than that I don't remember making any other significant change ?

Also, it seems like it's not fetching the updated repository, because I can see this commit direct-code-execution/ns-3-dce-umip@c594b82 which fixes the issue pushed to master.

I also see that it's probably loading cached source tree in the 4th build step (Restoring Cache) which is why in the 6th build step (download bake and code) even when highest level of verbosity(-vvv) is enabled, it doesn't seem to show git cloning outputs for any of the repos. This is something I took care of in my GActions pull request #118, to load from cache only if the HEAD has changed(and that too only for ns-3-dev because it's maintained very neatly and might take up significantly higher build time), but in our current circle-ci script we are loading from cache all the time, never downloading anything.

We could avoid this by removing caching all together, or resort to conditional caching just like I do in my GActions PR.

Sir, should I give it a try ?

@tomhenderson
Copy link
Collaborator

I would suggest to first disable all caching and then try to reintroduce conditional caching later, once it is stable.

Signed-off-by: Parth Pratim Chatterjee <[email protected]>
@teto
Copy link
Member

teto commented Jul 27, 2021

I've had some nightmarish issues with the cache so no issue disabling it on my side. I dont trust circleci to have it implemented well anyway. It would be nice to keep the ability to turn it on easily since having too long jobs can also hurt productivity (but right now the most important is for you to make progress without nay concern about the cache validity)

@ParthPratim
Copy link
Contributor Author

I think the build is passing now.
What would be the recommended next steps for now ?

@teto
Copy link
Member

teto commented Jul 27, 2021

You tell me :) should I merge ? Looks ok right now

@ParthPratim
Copy link
Contributor Author

😅 I guess we can if Tom Sir finds it ok too.

@tomhenderson
Copy link
Collaborator

+1; let me know if you want me to do the merge (I would squash and merge).

@teto teto merged commit 00c7561 into direct-code-execution:master Jul 27, 2021
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.

3 participants